Pr/oob sync - #113
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 103 files, which is 3 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (103)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds OOB management documentation, server-key-scoped sync and import behavior, OOB-aware UI states and badges, and DB-backed regression coverage. It also hardens multi-server API resolution, redirect handling, cache scoping, and validation flows across import, sync, and HTMX paths. ChangesOut-of-Band (OOB) Controller Management and Multi-Server Sync
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
341-398:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHide destructive delete controls in migrated mode.
The migrated modal adds per-row Move actions, but the surrounding checkbox column and
Delete Selected Interfacesfooter still render, leaving a donor-side destructive delete path next to migration. Guard the delete UI withnot migrated_to_markerso migrated mode is move-only.Proposed fix
- <th> - <input type="checkbox" id="select-all-netbox-interfaces" - class="form-check-input"> - </th> + {% if not migrated_to_marker %} + <th> + <input type="checkbox" id="select-all-netbox-interfaces" + class="form-check-input"> + </th> + {% endif %} <th>Interface Name</th> <th>Type</th> <th>Status</th> <th>Description</th> {% if migrated_to_marker %}<th class="text-end">Migrate</th>{% endif %} @@ - <td> - <input type="checkbox" name="interface_ids" value="{{ interface.id }}" - class="form-check-input netbox-interface-checkbox"> - </td> + {% if not migrated_to_marker %} + <td> + <input type="checkbox" name="interface_ids" value="{{ interface.id }}" + class="form-check-input netbox-interface-checkbox"> + </td> + {% endif %} @@ - {% if interface_sync.netbox_only_interfaces %} + {% if interface_sync.netbox_only_interfaces and not migrated_to_marker %} <button type="button" class="btn btn-danger" id="confirm-delete-interfaces"> <i class="mdi mdi-delete"></i> Delete Selected Interfaces </button> {% endif %}🤖 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/templates/netbox_librenms_plugin/_interface_sync_content.html` around lines 341 - 398, Hide the checkbox column and delete button when in migrated mode to prevent destructive delete actions. Wrap the table column containing the checkbox input with name="interface_ids" with a condition checking "not migrated_to_marker", and similarly wrap the "Delete Selected Interfaces" button with id="confirm-delete-interfaces" with the same condition so that the destructive delete UI only renders when not in migrated mode.netbox_librenms_plugin/import_utils/device_operations.py (1)
837-856:⚠️ Potential issue | 🟠 MajorCheck devices that reference the IP via
oob_ipbefore requiringassigned_object.Line 844 gates device lookup behind
existing_ip.assigned_object, but an IP can be set as a device's out-of-band management address without being assigned to any interface (i.e.,assigned_objectisNone). This causes the import to miss valid device references. According to NetBox architecture,Device.oob_ipis a direct ForeignKey toIPAddress, whileassigned_objectis a separate GenericForeignKey typically pointing to an Interface. These are independent relationships.Proposed fix
- if existing_ip and existing_ip.assigned_object: - device = ( - existing_ip.assigned_object.device - if hasattr(existing_ip.assigned_object, "device") - else None - ) + if existing_ip: + device = None + if existing_ip.assigned_object: + device = ( + existing_ip.assigned_object.device + if hasattr(existing_ip.assigned_object, "device") + else None + ) + if device is None: + device = Device.objects.filter(oob_ip=existing_ip).first() if 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/import_utils/device_operations.py` around lines 837 - 856, The current logic in the device lookup section only attempts to retrieve a device when existing_ip.assigned_object is not None, but an IP address can be directly referenced as a device's OOB management IP without being assigned to any interface. Add an additional check using IPAddress.objects filter to query for devices where Device.oob_ip references the existing_ip, and extract the device from that relationship if found. This check should be performed regardless of whether assigned_object exists, ensuring that devices referenced via the oob_ip ForeignKey are properly identified for the subsequent OOB type and link detection logic.
🤖 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/tests/test_coverage_sync_views.py`:
- Around line 1771-1777: The test helper method _obj_with_iface currently
creates a plain MagicMock object but passes object_type="device", causing
_build_interface_maps() to use the non-Device code path (obj.interfaces.all())
instead of the production Device path (Interface.objects.filter with
get_virtual_chassis_members). To fix this, set the mock object's __class__
attribute to Device so it has the proper Device shape, and instead of mocking
obj.interfaces.all.return_value, patch Interface.objects.filter to return the
expected interfaces list. This ensures the tests exercise the actual Device/VC
code path used in production.
In `@netbox_librenms_plugin/utils.py`:
- Around line 1550-1559: The set_device_ip_fk() function validates that the IP
is assigned to the correct device interface, but does not validate the IP family
(IPv4 vs IPv6) against the field being set. This allows invalid combinations
like setting an IPv6 address to primary_ip4 to be silently persisted since
save(update_fields=[...]) bypasses full_clean(). Add IP family validation in
set_device_ip_fk() after the interface ownership check: when field equals
"primary_ip4", verify that ip.version is 4; when field equals "primary_ip6",
verify that ip.version is 6; leave "oob_ip" without family restrictions. If the
IP family does not match, raise a ValueError with a descriptive message before
the setattr and save operations.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 837-856: The current logic in the device lookup section only
attempts to retrieve a device when existing_ip.assigned_object is not None, but
an IP address can be directly referenced as a device's OOB management IP without
being assigned to any interface. Add an additional check using IPAddress.objects
filter to query for devices where Device.oob_ip references the existing_ip, and
extract the device from that relationship if found. This check should be
performed regardless of whether assigned_object exists, ensuring that devices
referenced via the oob_ip ForeignKey are properly identified for the subsequent
OOB type and link detection logic.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Around line 341-398: Hide the checkbox column and delete button when in
migrated mode to prevent destructive delete actions. Wrap the table column
containing the checkbox input with name="interface_ids" with a condition
checking "not migrated_to_marker", and similarly wrap the "Delete Selected
Interfaces" button with id="confirm-delete-interfaces" with the same condition
so that the destructive delete UI only renders when not in migrated mode.
🪄 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: a8503a05-93ad-4d11-86d4-9a05c13fa615
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (80)
.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/bulk_import.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/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_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_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/modules.py
marcinpsk
added a commit
that referenced
this pull request
Jun 18, 2026
… lookup, migrated delete UI, Device-shaped tests) - utils.set_device_ip_fk: also enforce IP family (primary_ip4 must be IPv4, primary_ip6 IPv6), since update_fields bypasses Device.clean()'s family check. - import_utils/device_operations: find devices that reference the LibreNMS IP via Device.oob_ip even when the IP has no interface assignment (e.g. NAT'd OOB IP) — previously gated behind existing_ip.assigned_object. - _interface_sync_content.html: hide the bulk-delete UI (select-all + per-row checkboxes + Delete Selected Interfaces) in migrated mode so a donor is move-only. - test_coverage_sync_views: build REAL Device + Interface in the IP-sync helpers so process_ip_sync exercises the production Device branch of _build_interface_maps() (Interface.objects.filter(device__in=get_virtual_chassis_members(obj))) instead of the MagicMock obj.interfaces.all() path; _run_process now creates a real IP so the primary-set persists. Tests (all red->green for the 3 behavioural fixes): TestSetDeviceIpFkFamily, TestValidateDeviceForImportOOBIPFallback, migrated-mode delete-UI render tests.
marcinpsk
commented
Jun 18, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (2)
333-351:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGive the table checkboxes accessible names.
The select-all and per-row checkboxes have no label/accessible name, so screen-reader users cannot tell what each control selects. Add
aria-labels without changing the visual layout.Proposed accessibility fix
<th> <input type="checkbox" id="select-all-netbox-interfaces" - class="form-check-input"> + class="form-check-input" + aria-label="Select all NetBox-only interfaces"> </th> @@ <td> <input type="checkbox" name="interface_ids" value="{{ interface.id }}" - class="form-check-input netbox-interface-checkbox"> + class="form-check-input netbox-interface-checkbox" + aria-label="Select NetBox-only interface {{ interface.name }}"> </td>🤖 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/templates/netbox_librenms_plugin/_interface_sync_content.html` around lines 333 - 351, The checkboxes in the table lack accessible names, making them inaccessible to screen reader users. Add aria-label attributes to both the select-all checkbox with id select-all-netbox-interfaces and to each per-row checkbox with name attribute interface_ids in the tbody loop. The select-all checkbox should describe that it selects all interfaces, while the per-row checkboxes should reference the specific interface being selected (you can use the interface.name or interface.id to make each label unique). This adds the accessibility information without changing the visual layout.Source: Linters/SAST tools
319-324:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBranch the modal warning for migrated Move mode.
Line 367 switches migrated rows to a Move action, but the modal still warns that the interfaces will be permanently deleted. Render move-specific copy when
migrated_to_markeris set so the donor path does not look destructive.Proposed copy split
<div class="alert alert-warning"> <i class="mdi mdi-alert me-2"></i> + {% if migrated_to_marker %} + <strong>Warning:</strong> The following interfaces exist on the migrated donor but are not found in the LibreNMS + data. + Use <strong>Move</strong> to reassign them to {{ migrated_to_winner.name|default:"the winner device" }}. + {% else %} <strong>Warning:</strong> The following interfaces exist in NetBox but are not found in the LibreNMS data. Deleting these interfaces will permanently remove them from NetBox. This action cannot be undone. + {% endif %} </div>Also applies to: 367-379
🤖 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/templates/netbox_librenms_plugin/_interface_sync_content.html` around lines 319 - 324, The modal warning at lines 319-324 displays a permanent deletion message for all interfaces, but when migrated_to_marker is set (indicating a Move action at line 367), the warning should reflect that interfaces are being moved, not deleted. Conditionally render the alert content based on whether migrated_to_marker exists in the context: show move-specific copy that clarifies interfaces will be moved rather than permanently deleted when migrated_to_marker is present, and keep the current deletion warning when it is not present.
🤖 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/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Around line 333-351: The checkboxes in the table lack accessible names, making
them inaccessible to screen reader users. Add aria-label attributes to both the
select-all checkbox with id select-all-netbox-interfaces and to each per-row
checkbox with name attribute interface_ids in the tbody loop. The select-all
checkbox should describe that it selects all interfaces, while the per-row
checkboxes should reference the specific interface being selected (you can use
the interface.name or interface.id to make each label unique). This adds the
accessibility information without changing the visual layout.
- Around line 319-324: The modal warning at lines 319-324 displays a permanent
deletion message for all interfaces, but when migrated_to_marker is set
(indicating a Move action at line 367), the warning should reflect that
interfaces are being moved, not deleted. Conditionally render the alert content
based on whether migrated_to_marker exists in the context: show move-specific
copy that clarifies interfaces will be moved rather than permanently deleted
when migrated_to_marker is present, and keep the current deletion warning when
it is not present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d27a301-159e-4488-8720-a8ef5e5360be
📒 Files selected for processing (11)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.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). (3)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_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/_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/_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/_interface_sync_content.html
**/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 (29)
📚 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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_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/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_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/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_actions.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_actions.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_actions.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_actions.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_actions.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_actions.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_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/_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/_interface_sync_content.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_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/_interface_sync_content.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_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/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/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/device_operations.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-12T12:14:03.173Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_coverage_device_fields.py:643-673
Timestamp: 2026-03-12T12:14:03.173Z
Learning: In netbox_librenms_plugin/tests/test_coverage_device_fields.py, strengthen the CreateAndAssignPlatformView success-path tests (test_manufacturer_not_found around lines 642–672 and the adjacent test around lines 674–697) by asserting that the newly created Platform instance is assigned to the locked device object (mock_locked.platform is mock_platform_instance) and that mock_locked.save() is called. This validates the FK assignment and persistence, rather than only checking that messages.success() was invoked. This improvement is a known backlog item (low priority); do not re-raise as a new finding.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_device_fields.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
📚 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/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/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
🪛 ast-grep (0.43.0)
netbox_librenms_plugin/tests/test_coverage_actions.py
[error] 1550-1550: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 1579-1579: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 1860-1860: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 2324-2324: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 3296-3296: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
[warning] 335-335: No matching [ label ] tag found.
(input-requires-label)
[warning] 351-351: No matching [ label ] tag found.
(input-requires-label)
🔇 Additional comments (9)
netbox_librenms_plugin/import_utils/device_operations.py (1)
203-338: LGTM!Also applies to: 743-750, 860-871
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
17-42: LGTM!Also applies to: 101-104, 224-228, 399-403
netbox_librenms_plugin/utils.py (1)
1027-1048: LGTM!Also applies to: 1543-1550
netbox_librenms_plugin/views/imports/actions.py (1)
41-41: LGTM!Also applies to: 391-418, 481-640, 996-1015, 1355-1359, 2214-2378
netbox_librenms_plugin/tests/test_utils.py (1)
393-417: LGTM!Also applies to: 498-517, 519-535, 1169-1219
netbox_librenms_plugin/tests/test_coverage_device_fields.py (1)
1-15: LGTM!Also applies to: 42-198, 205-342, 351-510, 518-706, 713-1302, 1310-1507, 1578-1903, 2058-2522
netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py (1)
10-13: LGTM!Also applies to: 474-501, 887-985
netbox_librenms_plugin/tests/test_coverage_sync_views.py (1)
5-6: LGTM!Also applies to: 1765-1782, 1801-1803, 1822-1824, 2722-2822
netbox_librenms_plugin/tests/test_interface_sync_content_template.py (1)
80-98: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/test_coverage_base_views2.py`:
- Around line 555-567: The test test_vc_path_finds_by_librenms_id uses a
remote_port value ("Gi1/0/1") that matches the actual interface name, which
allows the test to pass even if the librenms_id lookup path is broken and the
method falls back to name-based lookup. Change the remote_port value in the link
dictionary to a different name (while preserving the VC slot prefix) to ensure
the test specifically exercises and validates the librenms_id lookup path rather
than accidentally passing through the name-based fallback mechanism.
🪄 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: f720d8ef-356b-4be2-b015-dd430c85562e
📒 Files selected for processing (8)
netbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_utils.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). (5)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.13)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (actions)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.py
🧠 Learnings (11)
📚 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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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/conftest.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.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_librenms_id.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.py
🔇 Additional comments (8)
netbox_librenms_plugin/tests/conftest.py (1)
96-105: LGTM!Also applies to: 108-112
netbox_librenms_plugin/tests/test_coverage_base_views.py (1)
17-20: LGTM!Also applies to: 545-627, 630-715, 732-762, 1986-2013, 2674-2707
netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
17-45: LGTM!Also applies to: 471-505, 513-535, 569-578, 594-622, 630-681, 1368-1416, 1895-1912, 1919-1941, 1949-1981
netbox_librenms_plugin/tests/test_coverage_sync_views.py (1)
7-7: LGTM!Also applies to: 249-319, 1069-1187, 1328-1434, 1707-1868, 2421-2571, 2609-2636
netbox_librenms_plugin/tests/test_coverage_sync_views2.py (1)
119-166: LGTM!Also applies to: 1015-1106, 1628-1723
netbox_librenms_plugin/tests/test_librenms_id.py (1)
5-586: LGTM!netbox_librenms_plugin/tests/test_sync_modules.py (1)
854-976: LGTM!netbox_librenms_plugin/tests/test_utils.py (1)
13-37: LGTM!Also applies to: 481-535
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/views/base/cables_view.py (1)
756-759:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same dual-name fallback in verify flow as in enrichment flow.
SingleCableVerifyView.post()keepslocal_port_altbut ignores it during name fallback (name=local_portonly). That breaks verify-row resolution for interfaces named from the alternate LibreNMS field, even thoughget_links_data()/enrich_local_port()now support that case.Suggested fix
- # If not found by librenms_id, try matching by name - if not interface and local_port: - interface = lookup_device.interfaces.filter(name=local_port).first() + # If not found by librenms_id, try displayed or alternate LibreNMS name + if not interface: + name_candidates = [n for n in (local_port, link_data.get("local_port_alt")) if n] + interface = lookup_device.interfaces.filter(name__in=name_candidates).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/views/base/cables_view.py` around lines 756 - 759, The interface name lookup in SingleCableVerifyView.post() only filters by local_port but should also attempt to match using local_port_alt as a fallback, consistent with how the enrichment flow handles this in enrich_local_port(). Modify the interfaces.filter(name=local_port).first() call to additionally check for local_port_alt if the primary local_port lookup returns no results, ensuring the verify flow supports the same dual-name fallback mechanism as the enrichment flow.netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (1)
536-559: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding explicit badge for
oob_already_linkedserial action.When
serial_action == 'oob_already_linked', the template falls through to the default "Serial match" badge (line 543), which doesn't clearly communicate that an OOB controller is already linked. The warning text appears in the warnings section, but users may miss that context.Consider adding an explicit badge case:
💡 Suggested badge for oob_already_linked
{% if validation.serial_action == 'oob_candidate' %} <span class="badge bg-purple-lt me-1"><i class="mdi mdi-chip"></i> OOB Detected</span> {% elif validation.serial_action == 'promote_to_host' %} <span class="badge bg-info-lt me-1"><i class="mdi mdi-server"></i> Host Detected</span> + {% elif validation.serial_action == 'oob_already_linked' %} + <span class="badge bg-info-lt me-1"><i class="mdi mdi-chip"></i> OOB Already Linked</span> {% else %} <span class="badge bg-warning-lt me-1"><i class="mdi mdi-barcode"></i> Serial match</span> {% endif %}🤖 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/templates/netbox_librenms_plugin/htmx/device_validation_details.html` around lines 536 - 559, The serial_action conditional block in the device_validation_details.html template is missing an explicit case for the 'oob_already_linked' action, causing it to fall through to the default "Serial match" badge which doesn't clearly indicate that an OOB controller is already linked. Add a new elif condition that checks for validation.serial_action == 'oob_already_linked' before the final else clause, and create a badge with an appropriate icon and message that clearly communicates the OOB is already linked (similar in style to the 'oob_candidate' badge case). This should be inserted after the 'promote_to_host' check and before the default else clause that displays the generic Serial match badge.
🤖 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/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 536-559: The serial_action conditional block in the
device_validation_details.html template is missing an explicit case for the
'oob_already_linked' action, causing it to fall through to the default "Serial
match" badge which doesn't clearly indicate that an OOB controller is already
linked. Add a new elif condition that checks for validation.serial_action ==
'oob_already_linked' before the final else clause, and create a badge with an
appropriate icon and message that clearly communicates the OOB is already linked
(similar in style to the 'oob_candidate' badge case). This should be inserted
after the 'promote_to_host' check and before the default else clause that
displays the generic Serial match badge.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 756-759: The interface name lookup in SingleCableVerifyView.post()
only filters by local_port but should also attempt to match using local_port_alt
as a fallback, consistent with how the enrichment flow handles this in
enrich_local_port(). Modify the interfaces.filter(name=local_port).first() call
to additionally check for local_port_alt if the primary local_port lookup
returns no results, ensuring the verify flow supports the same dual-name
fallback mechanism as the enrichment flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78f6dac3-dce3-4838-9c41-4d9cbdc5a1d6
📒 Files selected for processing (14)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_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_badge_contrast.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/imports/actions.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.13)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
🧰 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/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.py
**/import_validation_helpers.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/import_validation_helpers.py:import_validation_helpers.pymust provide validation state mutation functions:apply_role_to_validation(),apply_cluster_to_validation(),apply_rack_to_validation()for updating validation state when user selects a role/cluster/rack, andremove_validation_issue(),recalculate_validation_status()for maintaining issue list and overall status
import_validation_helpers.pymust provide helper functions:fetch_model_by_id()andextract_device_selections()for reading form data
Files:
netbox_librenms_plugin/import_validation_helpers.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/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
netbox_librenms_plugin/templates/**/+(*_sync|*_sync_base).html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Sync pages should extend
librenms_sync_base.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
**/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
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/device_validation_details.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/cables_view.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 (29)
📚 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_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_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/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_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/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.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_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.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_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.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_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.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_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.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_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.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_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_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/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/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/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/device_operations.py
📚 Learning: 2026-05-25T21:48:19.264Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html:12-12
Timestamp: 2026-05-25T21:48:19.264Z
Learning: In this plugin’s HTMX form templates, `hx-include` selectors that target toggle preference wrapper `<span>` element IDs (e.g., `#use-sysname-toggle`, `#strip-domain-toggle`, `#auto-create-ipam-toggle`) are intentional. Those wrapper spans contain both the hidden `off` fallback input and the checkbox; HTMX must include the wrapper so the correct value is serialized, including the unchecked/off state. Do not recommend changing `hx-include` to the checkbox IDs with the `-cb` suffix, since it would omit the hidden fallback and break unchecked/off state submission.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 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-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/cables_view.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/cables_view.pynetbox_librenms_plugin/views/imports/actions.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/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-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
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
🪛 ast-grep (0.43.0)
netbox_librenms_plugin/tests/test_coverage_actions.py
[error] 4752-4752: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 4754-4754: Lack of sanitization of user data
Context: HttpResponse("save failed", status=400)
Note: [CWE-20].
(http-response-from-request)
🔇 Additional comments (9)
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
4719-4769: Excellent atomic rollback integration test.This test properly exercises the real
transaction.atomic()+select_for_update()+ side-effect (Interface/IPAddress creation) paths in isolation from_save_device(), verifying that a mocked save failure triggersset_rollback(True)and discards all in-flight rows. The distinct host/OOB IDs (10 vs 17) and post-commit reload assertions strengthen confidence in the concurrent-write guard logic.netbox_librenms_plugin/tests/test_device_validation_details_template.py (1)
1-68: LGTM!netbox_librenms_plugin/import_validation_helpers.py (1)
254-273: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html (1)
41-95: LGTM!Also applies to: 141-141, 580-580
netbox_librenms_plugin/import_utils/device_operations.py (1)
303-311: LGTM!netbox_librenms_plugin/tests/test_import_validation_helpers.py (1)
401-415: LGTM!Also applies to: 431-465, 515-712
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (3)
264-264: LGTM!
406-414: LGTM!
586-638: LGTM!Also applies to: 671-719
marcinpsk
commented
Jun 19, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
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)
276-284:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve and surface the OOB fetch failure reason on hard refresh failure.
For an OOB-only mapping, the host fetch can set
_links_fetch_errorjust becauselibrenms_idis absent, while the real reason_prepare_context()returnsNoneis the failed OOB fetch. Since the OOB branch only stores a boolean, Line 620 can report the incidental host error instead of the controller failure. Store an_oob_links_fetch_errorstring and prefer it when_oob_links_fetch_failedis true.Proposed fix
# Reset per-call so a prior request's OOB failure doesn't leak into this one. self._oob_links_fetch_failed = False + self._oob_links_fetch_error = None @@ self._oob_links_fetch_failed = True + self._oob_links_fetch_error = "Unexpected response from LibreNMS (OOB links must be a list)." logger.warning( @@ self._oob_links_fetch_failed = True + self._oob_links_fetch_error = ( + (oob_data.get("error") or oob_data.get("message") or str(oob_data)) + if isinstance(oob_data, dict) + else str(oob_data) + ) logger.warning( @@ if context is None: # Surface the real fetch failure (auth/network/server) when there was one; # only fall back to the empty-result message when the device genuinely has no links. - if getattr(self, "_links_fetch_error", None): + if getattr(self, "_oob_links_fetch_failed", False) and getattr(self, "_oob_links_fetch_error", None): + messages.error(request, f"Failed to fetch OOB controller links from LibreNMS: {self._oob_links_fetch_error}") + elif getattr(self, "_links_fetch_error", None): messages.error(request, f"Failed to fetch links from LibreNMS: {self._links_fetch_error}") else: messages.error(request, "No links found in LibreNMS")Also applies to: 617-623
🤖 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 276 - 284, Store the actual OOB fetch error message alongside the boolean flag to ensure the correct failure reason is reported. In the exception handler where _oob_links_fetch_failed is set to True (around the logger.warning call), extract the error message from oob_data and store it in a new instance variable _oob_links_fetch_error using the same logic already used in the logger call (checking if oob_data is a dict and getting the "message" key, otherwise using oob_data directly). Then, in the error reporting logic around lines 620-623, check if _oob_links_fetch_failed is true and prefer returning _oob_links_fetch_error over _links_fetch_error to ensure the OOB controller failure is surfaced instead of an incidental host fetch error.
🤖 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 783-792: The hostname duplicate detection block (checking
existing_match_type == "hostname" and filtering Device.objects for duplicate
names) is currently nested inside a merge-candidate guard that requires
_serial_for_pair to be set. This means when a LibreNMS row has a duplicate
NetBox hostname but lacks a usable serial, the check never executes and an
arbitrary .first() match remains unchallenged. Move the entire hostname
uniqueness check block (the conditional starting with if
result.get("existing_match_type") == "hostname" and hostname and the subsequent
_hostname_peers and _ambiguous_current_side logic) outside and before the
_serial_for_pair merge-candidate guard so this validation always runs whenever a
hostname match is detected, regardless of serial availability. Apply the same
refactoring to the corresponding section at lines 859-873.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 276-284: Store the actual OOB fetch error message alongside the
boolean flag to ensure the correct failure reason is reported. In the exception
handler where _oob_links_fetch_failed is set to True (around the logger.warning
call), extract the error message from oob_data and store it in a new instance
variable _oob_links_fetch_error using the same logic already used in the logger
call (checking if oob_data is a dict and getting the "message" key, otherwise
using oob_data directly). Then, in the error reporting logic around lines
620-623, check if _oob_links_fetch_failed is true and prefer returning
_oob_links_fetch_error over _links_fetch_error to ensure the OOB controller
failure is surfaced instead of an incidental host fetch error.
🪄 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: 029cd5a8-d083-4f44-85c3-1cb5e39ddc4c
📒 Files selected for processing (22)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/device_status.pynetbox_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/htmx/device_validation_details.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_badge_contrast.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_device_operations.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
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/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
**/*.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.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
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
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/device_validation_details.html
**/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/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/ip_addresses_view.pynetbox_librenms_plugin/views/base/cables_view.py
🧠 Learnings (32)
📚 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/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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-05-25T21:48:19.264Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html:12-12
Timestamp: 2026-05-25T21:48:19.264Z
Learning: In this plugin’s HTMX form templates, `hx-include` selectors that target toggle preference wrapper `<span>` element IDs (e.g., `#use-sysname-toggle`, `#strip-domain-toggle`, `#auto-create-ipam-toggle`) are intentional. Those wrapper spans contain both the hidden `off` fallback input and the checkbox; HTMX must include the wrapper so the correct value is serialized, including the unchecked/off state. Do not recommend changing `hx-include` to the checkbox IDs with the `-cb` suffix, since it would omit the hidden fallback and break unchecked/off state submission.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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/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/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/device_operations.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/ip_addresses_view.pynetbox_librenms_plugin/views/base/cables_view.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/ip_addresses_view.pynetbox_librenms_plugin/views/base/cables_view.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/base/ip_addresses_view.pynetbox_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/ip_addresses_view.pynetbox_librenms_plugin/views/base/cables_view.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-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
🔇 Additional comments (31)
netbox_librenms_plugin/tests/conftest.py (1)
127-131: LGTM!netbox_librenms_plugin/tests/test_coverage_base_views.py (1)
2795-2813: LGTM!netbox_librenms_plugin/tests/test_device_validation_details_template.py (1)
129-186: LGTM!netbox_librenms_plugin/tests/test_interface_sync_content_template.py (1)
100-128: LGTM!netbox_librenms_plugin/tests/test_ipaddress_sync_content_template.py (1)
1-48: LGTM!netbox_librenms_plugin/tests/test_badge_contrast.py (1)
32-52: LGTM!Also applies to: 79-91
netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
465-493: LGTM!netbox_librenms_plugin/tests/test_coverage_sync_views2.py (1)
1644-1659: LGTM!netbox_librenms_plugin/tests/test_coverage_tables.py (1)
1468-1504: LGTM!netbox_librenms_plugin/tests/test_import_utils.py (1)
1295-1322: LGTM!Also applies to: 3645-3668
netbox_librenms_plugin/tests/test_utils.py (4)
418-444: LGTM!
445-455: LGTM!
493-546: LGTM!
1141-1193: LGTM!netbox_librenms_plugin/utils.py (1)
94-113: LGTM!netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
43-118: LGTM!Also applies to: 120-153, 155-179, 290-392, 414-456, 550-598
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (1)
1300-1330: LGTM!netbox_librenms_plugin/tables/device_status.py (1)
492-546: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (5)
17-46: LGTM!
47-88: LGTM!
106-106: LGTM!Also applies to: 226-230
323-331: LGTM!Also applies to: 341-364
377-394: LGTM!Also applies to: 409-409
netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.html (2)
9-36: LGTM!
95-99: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (6)
9-9: LGTM!
264-264: LGTM!
345-346: LGTM!
406-416: LGTM!
488-491: LGTM!Also applies to: 540-644
658-658: LGTM!Also applies to: 674-724
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (4)
netbox_librenms_plugin/tests/test_coverage_sync_views.py (1)
2522-2533:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on invalid VLAN group IDs instead of creating a global VLAN.
This test locks in a stale/tampered
vlan_group_10falling back togroup=None. That can persist a VLAN in the wrong scope;_handle_create_vlansshould skip/error that VID when the requestedVLANGroupdoes not exist.Suggested test expectation
- def test_invalid_group_id_falls_back_to_global(self): + def test_invalid_group_id_is_rejected(self): from ipam.models import VLAN @@ - # group id 999 does not exist → VLANGroup.DoesNotExist → falls back to a global VLAN. + # group id 999 does not exist → fail closed instead of creating a global VLAN. req = _make_request({"select": ["10"], "vlan_group_10": "999"}) - self._run(view, req, obj, cached_vlans) + mock_msg = self._run(view, req, obj, cached_vlans) - assert VLAN.objects.filter(vid=10, group__isnull=True).exists() + assert not VLAN.objects.filter(vid=10).exists() + mock_msg.error.assert_called()🤖 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/tests/test_coverage_sync_views.py` around lines 2522 - 2533, The test method test_invalid_group_id_falls_back_to_global is currently asserting that an invalid VLANGroup ID (999) results in creation of a global VLAN with group=None, which is the problematic behavior that should be prevented. Modify the test assertion to verify that the VLAN is NOT created when an invalid group ID is provided, rather than checking that it exists with a null group. This ensures the test validates the correct fail-safe behavior where invalid group IDs cause the VID to be skipped rather than falling back to an unsafe global scope.netbox_librenms_plugin/librenms_api.py (1)
32-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize legacy-mode server keys before returning the client.
build_librenms_api("ghost")only fails closed whenLibreNMSAPIraises. In legacy single-server mode it can still build a client while retaining the posted"ghost"asapi.server_key, sorebind_api_for_server()may cache/redirect under an unconfigured request value instead of the implicit default server.Suggested direction
else: # Fallback to legacy single-server configuration + # Legacy mode has only the implicit default server; don't let a stale/tampered + # request key become the cache/redirect discriminator. + server_key = "default" + self.server_key = server_key self.librenms_url = get_plugin_config("netbox_librenms_plugin", "librenms_url")Based on learnings, multi-server cache discriminators should use
api.server_key, soapi.server_keymust not retain unvalidated request values.🤖 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 32 - 34, The issue is that the `build_librenms_api` function can return a LibreNMSAPI client with an unvalidated server_key that was directly passed from the request (such as "ghost"), which can cause issues with caching in `rebind_api_for_server()`. After successfully creating the LibreNMSAPI instance, normalize the api.server_key to use the implicit default server key in legacy single-server mode instead of retaining the unconfigured posted value. This ensures the returned client has a validated server_key that won't corrupt the cache discriminator when used as a multi-server cache discriminator.Source: Learnings
netbox_librenms_plugin/views/base/modules_view.py (2)
431-461:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a server-side guard before cached OOB rows can drive module actions.
These OOB items are cached in the same inventory snapshot consumed by module sync actions; the provided
InstallBranchViewpath installs cached branch items byparent_indexwithout checking_source. A crafted POST can therefore attempt to install OOB-controller inventory onto the host despite the UI rendering those rows read-only. Reject_source == "oob"in the module action endpoints, or keep OOB rows out of the action cache.Suggested guard for the install-branch action path
branch_items = self._collect_branch(parent_index, cached_data, ignore_rules, device_serial, index_map) +if any(item.get("_source") == "oob" for item in branch_items): + messages.error(request, "OOB controller inventory is read-only from the host modules tab.") + return _modules_redirect_response(request, sync_url, server_key) + if not branch_items: messages.warning(request, "No installable items found in this branch.") return _modules_redirect_response(request, sync_url, 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/base/modules_view.py` around lines 431 - 461, Add server-side validation in the module action endpoints (such as InstallBranchView) to reject OOB inventory items before they can be used to perform actions. When processing module actions that work with cached inventory items identified by parent_index, check if the item has _source set to "oob" and reject the action if it does, since OOB items should remain read-only and not be installed onto the host despite being cached in the same inventory snapshot.
371-372:⚠️ Potential issue | 🟠 MajorNormalize main inventory indices before caching to match action lookup type.
The comment correctly identifies a type mismatch risk. LibreNMS API responses return SNMP indices as strings (as documented in
_try_int()at line 56), but the main inventory path only stamps_source = "main"without normalizing indices. Meanwhile, OOB inventory at lines 433–436 explicitly normalizes bothentPhysicalIndexandentPhysicalContainedInusing_try_int()before caching.When downstream action handlers cast
parent_indextoint(modules.py:688) and build lookup maps from cached inventory, string-keyed main items will not match integer lookups. This silently breaks parent resolution in install/branch actions for main device inventory.Apply the same normalization to main inventory indices as is done for OOB inventory:
Suggested fix
for item in inventory_data: item["_source"] = "main" # Normalize main inventory indices to match OOB normalization (lines 433-436) if (idx := _try_int(item.get("entPhysicalIndex"))) is not None: item["entPhysicalIndex"] = idx if (parent := _try_int(item.get("entPhysicalContainedIn"))) is not None: item["entPhysicalContainedIn"] = parentThis also applies to the fallback path at lines 425–437 when OOB fetch succeeds but the offset computation reads raw main indices—they should already be integers by that point.
🤖 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 371 - 372, In the main inventory loop where _source is being set to "main", add normalization of the entPhysicalIndex and entPhysicalContainedIn fields using the _try_int() function to convert them from strings to integers, matching the same normalization already applied to OOB inventory at lines 433-436. This ensures that when downstream handlers cast parent_index to int and perform lookups, the keys will match as integers rather than mismatched string values.
🤖 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/librenms_api.py`:
- Around line 32-34: The issue is that the `build_librenms_api` function can
return a LibreNMSAPI client with an unvalidated server_key that was directly
passed from the request (such as "ghost"), which can cause issues with caching
in `rebind_api_for_server()`. After successfully creating the LibreNMSAPI
instance, normalize the api.server_key to use the implicit default server key in
legacy single-server mode instead of retaining the unconfigured posted value.
This ensures the returned client has a validated server_key that won't corrupt
the cache discriminator when used as a multi-server cache discriminator.
In `@netbox_librenms_plugin/tests/test_coverage_sync_views.py`:
- Around line 2522-2533: The test method
test_invalid_group_id_falls_back_to_global is currently asserting that an
invalid VLANGroup ID (999) results in creation of a global VLAN with group=None,
which is the problematic behavior that should be prevented. Modify the test
assertion to verify that the VLAN is NOT created when an invalid group ID is
provided, rather than checking that it exists with a null group. This ensures
the test validates the correct fail-safe behavior where invalid group IDs cause
the VID to be skipped rather than falling back to an unsafe global scope.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 431-461: Add server-side validation in the module action endpoints
(such as InstallBranchView) to reject OOB inventory items before they can be
used to perform actions. When processing module actions that work with cached
inventory items identified by parent_index, check if the item has _source set to
"oob" and reject the action if it does, since OOB items should remain read-only
and not be installed onto the host despite being cached in the same inventory
snapshot.
- Around line 371-372: In the main inventory loop where _source is being set to
"main", add normalization of the entPhysicalIndex and entPhysicalContainedIn
fields using the _try_int() function to convert them from strings to integers,
matching the same normalization already applied to OOB inventory at lines
433-436. This ensures that when downstream handlers cast parent_index to int and
perform lookups, the keys will match as integers rather than mismatched string
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e52b9c36-60a6-4b09-9ada-a3a0bc726399
📒 Files selected for processing (46)
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_cable_verify.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_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.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_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_modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/utils.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/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/sync/device_fields.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
commented
Jun 20, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai — the outside-diff (body-only) findings here are recurring items already handled in earlier rounds (develop-owned ones live on the develop-hardening PR #116 per our stacked-PR convention):
These are re-surfaced every review (no resolved state); no new change needed. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
Owner
Author
|
@coderabbitai — the outside-diff (body-only) findings from the latest review are all already addressed in earlier rounds (CodeRabbit re-lists outside-diff findings each review regardless of prior replies). Verified against current code:
|
Owner
Author
|
@CodeRabbit review --base origin/develop --dir netbox_librenms_plugin/views |
|
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review --base origin/develop --dir netbox_librenms_plugin/views |
|
✅ Action performedReview finished.
|
…face picker The OOB detection UI (the OOB Detected validation panel and the OOB IP interface picker) was documented in prose only. Add dark-theme screenshots plus a 'Not seeing the panel?' note spelling out the two trigger conditions (OOB-looking os/hardware/name + a serial or management-IP match to an existing NetBox device), which is the common reason it doesn't appear. Device identifiers in the screenshots are blurred.
…ver_key resolutions
A rebase of the OOB-sync base commit re-applied a pre-fix modules.py, silently reverting
the earlier server_key validation: InstallModuleView and UpdateModuleInterfaceView lost
LibreNMSAPIMixin entirely (falling to a bare `(POST.get("server_key") or "").strip()` with
no fallback), and the four sibling views dropped the configured-key check back to
`... or self.librenms_api.server_key`.
With a blank/stale/forged posted server_key the bind was scoped under an empty or
unconfigured namespace: `if bind_item and server_key` short-circuited, so the module
installed but its interface was never bound to its LibreNMS port_id — no error surfaced.
Route all six resolutions (InstallModuleView, InstallBranchView, InstallSelectedView,
UpdateModuleInterfaceView, ModuleMismatchPreviewView, ReplaceModuleView) back through
resolve_posted_server_key and restore the mixin on the two that dropped it.
Tests:
- Assert LibreNMSAPIMixin stays in the MRO of both views, and that a blank posted
server_key falls back to the active server so the port_id bind still runs (red against
the unfixed views: bind called 0 times).
- The existing object.__new__-built view tests bypass __init__, so give them the
`_librenms_api` stub the mixin's __init__ would set, letting resolve_posted_server_key
fall back to the active server. The no-server-context test now drives that state via a
blank active-server key (a blank *posted* key alone no longer reaches it).
This was referenced Jul 31, 2026
Merged
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Out-of-band (OOB) controller support, plus the shared device-import base the later import PRs build on.
When a device's OOB controller (iDRAC/iLO/IPMI/Avocent) is registered as its own LibreNMS device, its inventory is merged read-only into the host's Interfaces, Cables and Modules tabs (shared-LOM detection via MAC, OOB
entPhysicalIndexoffset to avoid collisions). The import-validation flow gains an OOB-aware path: an incoming management IP that already equals a device'soob_ipis offered as an OOB link instead of a new import.Motivation / Problem
Feature. Base of the stack (review delta vs
develop).Scope of Change
How Was This Tested?
Risk Assessment
Additive. OOB rows are context only; sync/install never routes them onto the host. Non-OOB devices unchanged.
Backwards Compatibility
Summary by CodeRabbit
New Features
Bug Fixes
Documentation