Feat/device merge - #114
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR hardens LibreNMS device matching and refresh validation, adds promotion, merge, and migration transfer workflows, standardizes server-scoped partial rendering, and expands coverage for permissions, caching, serial normalization, OOB handling, and migration state. ChangesImport validation and migration
Synchronization and rendering
Sequence Diagram(s)sequenceDiagram
participant Validation as Device validation
participant Actions as Import actions
participant Marker as Migration markers
participant SyncView as Sync views
Validation->>Actions: select promotion or merge operation
Actions->>Marker: create server-scoped migration marker
Marker->>SyncView: expose migrated donor and winner context
SyncView->>Actions: submit interface or IP move
Actions->>Marker: revalidate marker and persist transfer
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
marcinpsk
force-pushed
the
feat/device-merge
branch
4 times, most recently
from
June 19, 2026 17:58
dd5aa2b to
ee77eef
Compare
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: 9
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/templates/netbox_librenms_plugin/_ipaddress_sync_content.html (1)
19-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHide the
Set Primary IPswitch in migrated mode.The form is removed in migrated mode, so this switch renders as an active option with nowhere to submit and no effect. Gate it with
not migrated_to_marker.Proposed fix
<button type="submit" class="btn btn-primary"> <span class="spinner spinner-border d-none" id="sync-spinner"></span> <span>Sync Selected IP Addresses</span> </button> {% endif %} + {% if not migrated_to_marker %} <div class="form-check form-switch mb-0"> <input type="hidden" name="set-primary-ip-toggle" value="off"> <input class="form-check-input" type="checkbox" id="set-primary-ip-toggle-cb" @@ Set Primary IP </label> </div> + {% 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/_ipaddress_sync_content.html` around lines 19 - 34, The Set Primary IP switch is not hidden in migrated mode even though the form is removed. Wrap the entire div element with class form-check form-switch (which contains the set-primary-ip-toggle input and its label) with the same {% if not migrated_to_marker %} condition that gates the submit button above it, ensuring the switch is only displayed when not migrated_to_marker is true.netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
156-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on duplicate LibreNMS interface IDs before IP matching.
This map overwrites earlier interfaces when two NetBox interfaces share the same server-scoped LibreNMS ID, so
_add_interface_info_to_ip()can bind an IP row to whichever interface was iterated last instead of falling back to name matching. Drop ambiguous IDs from the map.Proposed fix
- interfaces_by_librenms_id = {} + interfaces_by_librenms_id = {} + ambiguous_librenms_ids = set() for interface in all_interfaces: lib_id = get_librenms_device_id(interface, server_key, auto_save=False) if lib_id is not None: - interfaces_by_librenms_id[str(lib_id)] = interface + lib_id_key = str(lib_id) + if lib_id_key in ambiguous_librenms_ids: + continue + if lib_id_key in interfaces_by_librenms_id: + ambiguous_librenms_ids.add(lib_id_key) + interfaces_by_librenms_id.pop(lib_id_key, None) + continue + interfaces_by_librenms_id[lib_id_key] = interface🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/base/ip_addresses_view.py` around lines 156 - 168, The `_prefetch_netbox_data` method builds an `interfaces_by_librenms_id` dictionary that overwrites entries when two interfaces share the same LibreNMS ID for a given server_key, creating ambiguity when matching IPs to interfaces. Track which LibreNMS IDs appear more than once during the iteration through all_interfaces, and remove those duplicate IDs from the `interfaces_by_librenms_id` map so that downstream calls to `_add_interface_info_to_ip()` will safely fall back to name-based matching instead of using an ambiguous mapping. This ensures the code fails closed by rejecting ambiguous matches rather than silently picking whichever interface was iterated last.netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
37-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHide sync-only controls when the donor form is removed.
In migrated mode the
<form>and submit button are gone, but the “Exclude from Sync” checkboxes still render as active controls and cannot be submitted. Gate that section with the submit button.Proposed fix
<button type="submit" class="btn btn-primary"> <span class="spinner spinner-border d-none" id="sync-spinner"></span> <span>Sync Selected Interfaces</span> </button> {% endif %} @@ + {% if not migrated_to_marker %} <div class="ms-auto d-flex align-items-center"> <div class="exclude-columns-section d-flex align-items-center gap-2 me-2"> <h6 class="mb-0">Exclude from Sync:</h6> @@ </div> </div> + {% 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 37 - 85, The "Exclude from Sync" checkboxes in the exclude-columns-section div are still rendered even when the form and submit button are hidden in migrated mode, creating non-functional orphaned controls. Wrap the exclude-columns-section div (containing all the exclude checkboxes for Type, Speed, VLANs, MAC, MTU, Enabled, and Description) with the same {% if not migrated_to_marker %} conditional that gates the submit button, so these controls only appear when there's an actual form to submit.netbox_librenms_plugin/views/base/vlan_table_view.py (1)
98-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the VLAN payload before caching it.
A truthy
successwith a non-list or non-dict row payload gets cached, thencompare_vlans()iterates it and callsvlan.get(...), causing a 500 on the fragment render.🔧 Proposed fix
success, vlans_data = self.librenms_api.get_device_vlans(self.librenms_id) if not success: return False, f"Failed to fetch VLANs: {vlans_data}" +if not isinstance(vlans_data, list) or not all(isinstance(vlan, dict) for vlan in vlans_data): + return False, "Unexpected response from LibreNMS (malformed VLAN payload)." # Cache VLANs (scoped to the POST-resolved server when provided). server_key = server_key or self.librenms_api.server_key🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/base/vlan_table_view.py` around lines 98 - 103, Add payload validation after checking the success response from self.librenms_api.get_device_vlans() but before caching the vlans_data. Verify that vlans_data is a list or dict containing the expected structure, and if not, return False with a descriptive error message. This prevents compare_vlans() from attempting to call .get() on non-dictionary items which would cause a runtime error during fragment rendering.
🤖 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 672-693: The hostname-match case in the elif existing_device block
sets existing_librenms_link but does not propagate the link state into the
warning message like the hostname-match VM case above it does. After setting
result["existing_librenms_link"] in the elif existing_device branch, apply the
same link_note logic used in the VM branch: check if existing_librenms_link has
a host_id (link to specific device), oob_id (OOB controller link), or neither
(not linked), construct a link_note variable accordingly, and append a warning
message that includes this link state information so the modal displays
consistent information about the device's current LibreNMS linkage.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Around line 367-384: The hx-confirm modal warning text for the move interface
button in the migrated_to_winner block needs to be updated to accurately
describe the transfer operation rather than warning about permanent deletion.
Update the hx-confirm attribute text to clearly communicate that this action
will move/reassign the interface to another device, ensuring the confirmation
message aligns with the actual transfer flow operation being performed rather
than any deletion semantics.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html`:
- Around line 13-24: The HTMX buttons for device and VM interface sync are not
including the active server_key in their POST requests. The
BaseInterfaceTableView.post() method retrieves server_key from request.POST, but
the hx-include attributes on both the device_interface_sync button and
vm_interface_sync button only include interface_name_field. Update the
hx-include attributes on both buttons to also include the server_key field so
that the active server key is properly posted with the refresh request, ensuring
correct cache scoping and redirects.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 422-423: The list item describing "Moved to winner" currently
states the OOB IP moves when the winner has no OOB IP yet, but this does not
fully capture the actual transfer logic. Update the text in the first list item
to clarify that the OOB IP only moves to the winner when both conditions are
met: the winner has no OOB IP AND the donor IP is already assigned to a
winner-owned interface; otherwise, the OOB IP stays on the donor and must be
re-homed later. Make the wording precise to match the actual merge behavior
described in the review comment.
- Around line 629-633: The conditional logic that checks if
validation.existing_librenms_link.host_id is set only handles cases where a
device has a main device link. However, a device can be linked to LibreNMS as an
OOB-only controller where host_id is empty but oob_id is set, and the current
condition incorrectly falls through to "not linked" for these cases. Update the
condition from checking only host_id to checking whether either host_id or
oob_id exists, so that OOB-only linked devices properly display their OOB
information instead of showing "not linked".
In `@netbox_librenms_plugin/tests/conftest.py`:
- Around line 127-131: The `make_module_type_with_bays()` function only creates
ModuleBayTemplate entries when the ModuleType is newly created (when
created=True), which causes tests to be order-dependent. If the same model is
reused with different bay_names, the new templates are silently skipped. Fix
this by moving the bay template creation logic outside of the `if created:`
block so that bay templates are processed and created every time the function is
called, ensuring the helper is additive and handles both new and existing module
types correctly.
In `@netbox_librenms_plugin/tests/test_badge_contrast.py`:
- Around line 35-47: The _CLASS_ATTR regex pattern in the _bare_badge_offenders
function currently only matches class attributes with double quotes on a single
line, causing it to miss badges with single quotes or multiline class attributes
that span across multiple lines. Update the _CLASS_ATTR regex pattern to match
both single and double quoted class attributes, and modify the parsing logic in
_bare_badge_offenders to handle multiline class attributes by either
concatenating consecutive lines or adjusting the regex to work with the
re.DOTALL flag to match across newlines.
In `@netbox_librenms_plugin/utils.py`:
- Around line 94-109: The call to members.all() in the return statement of
get_virtual_chassis_members is not guarded, so enumeration errors will bubble up
instead of returning the documented fallback [device] value. Wrap the
list(members.all()) call in a try-except block to catch any errors that occur
during member enumeration, and return [device] in the except handler to preserve
the documented fallback behavior when the members cannot be enumerated.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 127-128: The error extraction logic in the assignment to
`_links_fetch_error` only checks for the "error" key in the data dictionary, but
LibreNMS can return error information under a "message" key instead, causing
errors to not be captured. When `success` is `False`, modify the error
extraction logic to check for both "error" and "message" keys in the data
dictionary, falling back to "message" if "error" is not present, ensuring that
error information is properly preserved regardless of which key LibreNMS uses.
---
Outside diff comments:
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Around line 37-85: The "Exclude from Sync" checkboxes in the
exclude-columns-section div are still rendered even when the form and submit
button are hidden in migrated mode, creating non-functional orphaned controls.
Wrap the exclude-columns-section div (containing all the exclude checkboxes for
Type, Speed, VLANs, MAC, MTU, Enabled, and Description) with the same {% if not
migrated_to_marker %} conditional that gates the submit button, so these
controls only appear when there's an actual form to submit.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.html`:
- Around line 19-34: The Set Primary IP switch is not hidden in migrated mode
even though the form is removed. Wrap the entire div element with class
form-check form-switch (which contains the set-primary-ip-toggle input and its
label) with the same {% if not migrated_to_marker %} condition that gates the
submit button above it, ensuring the switch is only displayed when not
migrated_to_marker is true.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 156-168: The `_prefetch_netbox_data` method builds an
`interfaces_by_librenms_id` dictionary that overwrites entries when two
interfaces share the same LibreNMS ID for a given server_key, creating ambiguity
when matching IPs to interfaces. Track which LibreNMS IDs appear more than once
during the iteration through all_interfaces, and remove those duplicate IDs from
the `interfaces_by_librenms_id` map so that downstream calls to
`_add_interface_info_to_ip()` will safely fall back to name-based matching
instead of using an ambiguous mapping. This ensures the code fails closed by
rejecting ambiguous matches rather than silently picking whichever interface was
iterated last.
In `@netbox_librenms_plugin/views/base/vlan_table_view.py`:
- Around line 98-103: Add payload validation after checking the success response
from self.librenms_api.get_device_vlans() but before caching the vlans_data.
Verify that vlans_data is a list or dict containing the expected structure, and
if not, return False with a descriptive error message. This prevents
compare_vlans() from attempting to call .get() on non-dictionary items which
would cause a runtime error during fragment rendering.
🪄 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: 93eb3eff-6b0f-4f25-9318-a6469055779b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.gitignoredocs/SUMMARY.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/custom_field.mddocs/usage_tips/oob_management.mdmkdocs.ymlnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/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_badge_contrast.pynetbox_librenms_plugin/tests/test_cable_sync_content_template.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_module_sync_content_template.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
force-pushed
the
feat/device-merge
branch
from
June 20, 2026 05:17
ee77eef to
ff356c5
Compare
Owner
Author
|
@coderabbitai Addressed the outside-diff findings:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List CodeRabbit review comments on PR `#114`
gh api repos/marcinpsk/netbox-librenms-plugin/pulls/114/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, line: .original_line, body_start: .body[:120]}'Length of output: 2959 🏁 Script executed: #!/bin/bash
# List CodeRabbit issue comments on PR `#114`
gh api repos/marcinpsk/netbox-librenms-plugin/issues/114/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body_start: .body[:300]}'Length of output: 1016
🐇 ✓ ✏️ Learnings added
|
marcinpsk
force-pushed
the
feat/device-merge
branch
from
June 20, 2026 08:00
9b11f49 to
1f7c042
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
force-pushed
the
feat/device-merge
branch
from
June 20, 2026 09:54
1f7c042 to
e9af2f9
Compare
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 (5)
netbox_librenms_plugin/views/imports/actions.py (2)
2313-2329:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSerialize the duplicate LibreNMS-id check.
find_by_librenms_id()is still an unlocked read, so two concurrent OOB/promote requests can both pass the conflict check and persist the same LibreNMS id on different devices. The row lock only protectsexisting_device, not the competing owner row. The same race exists inPromoteToHostViewbelow.🤖 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/imports/actions.py` around lines 2313 - 2329, The duplicate LibreNMS-id check using `find_by_librenms_id()` is an unlocked read, allowing two concurrent OOB/promote requests to both pass the conflict check and persist the same LibreNMS id on different devices. Serialize this check by locking the Device rows that could potentially own the LibreNMS id during the conflict validation, using a database-level lock (such as select_for_update) to prevent concurrent requests from bypassing the check. Apply this same fix to the identical race condition in PromoteToHostView's host_conflict guard.
2534-2556:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCompare the assigned object type, not just the pk.
existing.assigned_object.pk == selected_iface_pkcan treat a different model with the same pk as the selected Interface as “already on the chosen interface”. That skips thechange_ipaddresswarning and only fails later in_attach_oob_ip(). Add a type check here.🤖 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/imports/actions.py` around lines 2534 - 2556, The comparison of assigned object pk with selected_iface_pk only validates the primary key match but does not verify that the assigned_object is actually an Interface type, allowing different model types with the same pk to be incorrectly treated as already assigned to the selected interface. In the assignment of already_on_selected_iface, add a type check to ensure that existing.assigned_object is an instance of Interface in addition to comparing the pk values, so that both the correct type and matching pk are verified before determining the interface is already selected.netbox_librenms_plugin/views/base/vlan_table_view.py (1)
98-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate VLAN payload shape before caching it.
compare_vlans()later callsvlan.get(...)for every cached item, so a successful LibreNMS response with a dict or a list containing non-dicts can persist a cache entry that crashes the next render instead of surfacing a fetch error.🛡️ Proposed validation before cache write
success, vlans_data = self.librenms_api.get_device_vlans(self.librenms_id) if not success: return False, f"Failed to fetch VLANs: {vlans_data}" + if not isinstance(vlans_data, list) or not all(isinstance(vlan, dict) for vlan in vlans_data): + return False, "Failed to fetch VLANs: malformed VLAN payload" # Cache VLANs (scoped to the POST-resolved server when provided). server_key = server_key or self.librenms_api.server_key🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/base/vlan_table_view.py` around lines 98 - 103, After the successful response check for get_device_vlans in this method, add validation to ensure the vlans_data payload has the expected shape (a list of dictionaries) before caching it. If vlans_data is not in the correct format, return a failure status with an appropriate error message instead of proceeding to cache the data. This prevents invalid data from being cached and causing crashes later when compare_vlans() attempts to call dot-notation get methods on the cached items.netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
226-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
get_port_by_id()payload shape before indexing it.
get_port_by_id()is still an external boundary here:success=Truewith{"port": "bad"},{"port": {}}, or a non-dict payload can raise or cache a non-dictport_info, and line 97 then calls.get()on it during refresh/backfill.Proposed fix
if port_id not in port_data_cache: success, port_data = self.librenms_api.get_port_by_id(port_id) - if success and "port" in port_data and port_data["port"]: - port_data_cache[port_id] = port_data["port"][0] + port_rows = port_data.get("port") if success and isinstance(port_data, dict) else None + if isinstance(port_rows, list) and port_rows and isinstance(port_rows[0], dict): + port_data_cache[port_id] = port_rows[0] else: port_data_cache[port_id] = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/base/ip_addresses_view.py` around lines 226 - 229, The code in the get_port_by_id() response handling does not validate the shape of the payload before indexing and caching it. Even when success is True and "port" exists in port_data, the value at port_data["port"] could be a non-list value or contain non-dict items, which would cause failures later when line 97 calls .get() on the cached port_info. Add validation to ensure that port_data["port"] is a list with at least one element and that the first element at index [0] is a dictionary before storing it in port_data_cache.netbox_librenms_plugin/import_utils/device_operations.py (1)
599-604:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winForce Device mode when a Device match wins.
The VM match branch sets
result["import_as_vm"] = True, but Device match branches leave a user-selected VM mode unchanged. A LibreNMS row already mapped to a NetBoxDevicecan then flow through the later VM validation/UI path after Line 980 refreshesimport_as_vmfrom the result.Proposed fix
result["existing_device"] = existing_device result["existing_match_type"] = "librenms_id" + result["import_as_vm"] = False result["can_import"] = Falseresult["existing_device"] = existing_device result["existing_match_type"] = "hostname" + result["import_as_vm"] = False # Surface the current host/OOB linkage so a hostname-matched device thatAlso applies to: 713-719
🤖 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 599 - 604, In the device matching branch where existing_device is found by librenms_id, explicitly set the import mode to device by adding result["import_as_vm"] = False alongside the existing assignments of result["existing_device"], result["existing_match_type"], and result["can_import"]. Apply the same fix to all other device match branches (including the block referenced at lines 713-719) to ensure that whenever a device match wins, the import mode is forced to False, preventing the result from inheriting any user-selected VM mode that would be problematic when the result is processed later.
🤖 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_tables.py`:
- Around line 1461-1496: The test method
test_malformed_paired_oob_id_does_not_render_host_state currently only verifies
that certain strings are absent from the rendered output (negative assertions
for "Linked as host" and "`#bad`"), but it does not verify that the expected
fallback state is actually present. Add a positive assertion to confirm that the
generic details button or fallback state is properly rendered in the result
string when the paired oob_id is malformed, ensuring the test fails if rendering
produces an unintended empty state rather than the correct default behavior.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 599-604: In the device matching branch where existing_device is
found by librenms_id, explicitly set the import mode to device by adding
result["import_as_vm"] = False alongside the existing assignments of
result["existing_device"], result["existing_match_type"], and
result["can_import"]. Apply the same fix to all other device match branches
(including the block referenced at lines 713-719) to ensure that whenever a
device match wins, the import mode is forced to False, preventing the result
from inheriting any user-selected VM mode that would be problematic when the
result is processed later.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 226-229: The code in the get_port_by_id() response handling does
not validate the shape of the payload before indexing and caching it. Even when
success is True and "port" exists in port_data, the value at port_data["port"]
could be a non-list value or contain non-dict items, which would cause failures
later when line 97 calls .get() on the cached port_info. Add validation to
ensure that port_data["port"] is a list with at least one element and that the
first element at index [0] is a dictionary before storing it in port_data_cache.
In `@netbox_librenms_plugin/views/base/vlan_table_view.py`:
- Around line 98-103: After the successful response check for get_device_vlans
in this method, add validation to ensure the vlans_data payload has the expected
shape (a list of dictionaries) before caching it. If vlans_data is not in the
correct format, return a failure status with an appropriate error message
instead of proceeding to cache the data. This prevents invalid data from being
cached and causing crashes later when compare_vlans() attempts to call
dot-notation get methods on the cached items.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 2313-2329: The duplicate LibreNMS-id check using
`find_by_librenms_id()` is an unlocked read, allowing two concurrent OOB/promote
requests to both pass the conflict check and persist the same LibreNMS id on
different devices. Serialize this check by locking the Device rows that could
potentially own the LibreNMS id during the conflict validation, using a
database-level lock (such as select_for_update) to prevent concurrent requests
from bypassing the check. Apply this same fix to the identical race condition in
PromoteToHostView's host_conflict guard.
- Around line 2534-2556: The comparison of assigned object pk with
selected_iface_pk only validates the primary key match but does not verify that
the assigned_object is actually an Interface type, allowing different model
types with the same pk to be incorrectly treated as already assigned to the
selected interface. In the assignment of already_on_selected_iface, add a type
check to ensure that existing.assigned_object is an instance of Interface in
addition to comparing the pk values, so that both the correct type and matching
pk are verified before determining the interface is already selected.
🪄 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: b344b427-5a58-437d-bb03-ca9ff84d3260
📒 Files selected for processing (58)
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/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_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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_modules.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/sync/device_fields.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
commented
Jun 20, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (3)
netbox_librenms_plugin/views/base/ip_addresses_view.py (2)
592-592:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the active server key as the JSON fallback.
Hard-coding
"default"means a verify request that omitsserver_keywhile the user is on another LibreNMS server can read the wrong cache namespace and render status from another server’s snapshot.🐛 Proposed fix
- server_key = data.get("server_key") or "default" + server_key = data.get("server_key") or self.librenms_api.server_keyBased on learnings, “When handling POST requests, read the POST-scoped
server_keywith a fallback toself.librenms_api.server_key; use this POST-scoped key for cache-key scoping.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/base/ip_addresses_view.py` at line 592, The server_key assignment in ip_addresses_view.py is using a hard-coded "default" string as the fallback, which can cause cache namespace collisions across different LibreNMS servers. Replace the hard-coded "default" fallback with self.librenms_api.server_key so that when server_key is not provided in the POST data, the currently active server's key is used instead, ensuring proper cache scoping and preventing data from being read from the wrong server's namespace.Source: Learnings
224-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
get_port_by_id()before indexingport.A truthy LibreNMS response with
port=None, a dict, a scalar, or a non-dict first item can still raise here or makeport_info.get(...)fail later during IP refresh.🐛 Proposed fix
if port_id not in port_data_cache: success, port_data = self.librenms_api.get_port_by_id(port_id) - if success and "port" in port_data and port_data["port"]: - port_data_cache[port_id] = port_data["port"][0] + port_rows = port_data.get("port") if success and isinstance(port_data, dict) else None + if isinstance(port_rows, list) and port_rows and isinstance(port_rows[0], dict): + port_data_cache[port_id] = port_rows[0] else: port_data_cache[port_id] = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/base/ip_addresses_view.py` around lines 224 - 229, The `_get_port_info` method checks if `port_data["port"]` is truthy but doesn't validate its type or structure before indexing with `[0]`. The response could be a dict, scalar, or list with non-dict items, causing index errors or failures later. Before accessing `port_data["port"][0]`, add validation to ensure it is a list, is not empty, and has a dict as its first element. Only proceed with caching if all these conditions are met.netbox_librenms_plugin/views/base/interfaces_view.py (1)
153-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve migrated-donor state on the stale-server branch.
Line 156 redirects without the POSTed
server_keyorbuild_migrated_context(). For an HTMX refresh on a migrated donor whose server key was removed, this can return UI withoutmigrated_to_markerand re-enable sync controls that the partial is supposed to suppress.🐛 Proposed fix
- post_server_key = self.rebind_api_for_server(request.POST.get("server_key")) + posted_server_key = request.POST.get("server_key") + post_server_key = self.rebind_api_for_server(posted_server_key) if post_server_key is None: messages.error(request, "Selected LibreNMS server is no longer configured.") - return redirect(self.get_redirect_url(obj)) + return render( + request, + self.partial_template_name, + { + "interface_sync": { + "object": obj, + "table": None, + "cache_expiry": None, + "server_key": None, + }, + "interface_name_field": interface_name_field, + **build_migrated_context(obj, posted_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/interfaces_view.py` around lines 153 - 156, The redirect at the end of the error handling block (when post_server_key is None) does not preserve the migrated-donor state, causing HTMX refreshes to return UI without the migrated_to_marker and potentially re-enable suppressed sync controls. Modify the redirect call to preserve the migrated state by including the POSTed server_key parameter and ensuring build_migrated_context() is called to maintain the proper context when redirecting back to get_redirect_url(obj). This will prevent the UI from losing track of the migrated-donor relationship when the server key becomes stale.
🤖 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 808-823: In the duplicate-match handling block where _dup_current
is True, the existing_match_type variable is not being cleared alongside
serial_action, which allows the device_validation_details.html template to still
render the Link to LibreNMS form. Add a line to clear or reset the
existing_match_type in the result dictionary (set it to None or an empty string)
in the same location where you're already setting serial_action to None and
other blocking flags, so that the template will not render the link action form
for duplicate-flagged rows.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 449-462: The conditional logic in both the host candidate and oob
candidate sections does not properly handle OOB-only linked devices. Currently,
when librenms_link.host_id is empty, the code falls through to "not linked to
LibreNMS" without checking if librenms_link.oob_id exists. To fix this, modify
the condition in both label sections (one for the "Host name match" badge and
one for the "Serial match (OOB)" badge) to add an additional else-if check that
looks for oob_id when host_id is empty. Display appropriate text indicating the
OOB-only link status (showing the OOB device ID and type if available) before
falling through to the "not linked to LibreNMS" message. This ensures that
devices with OOB-only links are labeled correctly in the merge candidate
descriptions.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 153-156: The redirect at the end of the error handling block (when
post_server_key is None) does not preserve the migrated-donor state, causing
HTMX refreshes to return UI without the migrated_to_marker and potentially
re-enable suppressed sync controls. Modify the redirect call to preserve the
migrated state by including the POSTed server_key parameter and ensuring
build_migrated_context() is called to maintain the proper context when
redirecting back to get_redirect_url(obj). This will prevent the UI from losing
track of the migrated-donor relationship when the server key becomes stale.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Line 592: The server_key assignment in ip_addresses_view.py is using a
hard-coded "default" string as the fallback, which can cause cache namespace
collisions across different LibreNMS servers. Replace the hard-coded "default"
fallback with self.librenms_api.server_key so that when server_key is not
provided in the POST data, the currently active server's key is used instead,
ensuring proper cache scoping and preventing data from being read from the wrong
server's namespace.
- Around line 224-229: The `_get_port_info` method checks if `port_data["port"]`
is truthy but doesn't validate its type or structure before indexing with `[0]`.
The response could be a dict, scalar, or list with non-dict items, causing index
errors or failures later. Before accessing `port_data["port"][0]`, add
validation to ensure it is a list, is not empty, and has a dict as its first
element. Only proceed with caching if all these conditions are met.
🪄 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: 253670f9-a80d-4e76-bcfc-ca9de9fcf452
📒 Files selected for processing (25)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_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_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_sync_modules.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/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
force-pushed
the
feat/device-merge
branch
from
June 20, 2026 14:03
e9af2f9 to
4012721
Compare
marcinpsk
commented
Jun 20, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
netbox_librenms_plugin/tests/test_vlan_sync.py (1)
613-625:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake failure-path cache-eviction assertions exclusive to the POSTed server scope.
assert_any_call("prod:...")confirms expected deletes happened, but still passes ifdefault:...keys are also deleted. Add negative assertions to pin cross-server isolation in both tests.Suggested assertion additions
def test_missing_librenms_id_evicts_scoped_cache(self): view, mock_cache = self._run(librenms_id=None, vlans_result=(True, [])) @@ mock_cache.delete.assert_any_call("prod:vlans") mock_cache.delete.assert_any_call("prod:vlans:last") + deleted = [c.args[0] for c in mock_cache.delete.call_args_list] + assert "default:vlans" not in deleted + assert "default:vlans:last" not in deleted def test_fetch_failure_evicts_scoped_cache(self): view, mock_cache = self._run(librenms_id=10, vlans_result=(False, "boom")) @@ mock_cache.delete.assert_any_call("prod:vlans") mock_cache.delete.assert_any_call("prod:vlans:last") + deleted = [c.args[0] for c in mock_cache.delete.call_args_list] + assert "default:vlans" not in deleted + assert "default:vlans:last" not in deletedBased on learnings, multi-server cache behavior in this repository must stay server_key-scoped to prevent cross-server cache bleed.
🤖 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_vlan_sync.py` around lines 613 - 625, In both test_missing_librenms_id_evicts_scoped_cache and test_fetch_failure_evicts_scoped_cache, the current assertions using mock_cache.delete.assert_any_call only verify that prod-scoped cache keys are deleted but do not verify that default-scoped or other server-scoped keys are NOT deleted. Add negative assertions after the existing assert_any_call statements to ensure that mock_cache.delete was not called with default:vlans or default:vlans:last keys, thereby enforcing server-key isolation and preventing unintended cross-server cache bleed in both test methods.Source: Learnings
netbox_librenms_plugin/views/base/modules_view.py (1)
329-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
librenms_idbefore using it in fetch/cache flow.At Line 329 and Line 527, this view uses raw
get_librenms_id()values. At Line 330,if not self.librenms_idis a truthiness gate, and Line 528 compares an unnormalized value against cached fingerprint data. This can mis-handle malformed/string/bool CF values and cause incorrect fetch behavior or repeated cache invalidation.💡 Suggested fix
@@ - self.librenms_id = self.librenms_api.get_librenms_id(sync_device) - if not self.librenms_id: + self.librenms_id = coerce_librenms_id(self.librenms_api.get_librenms_id(sync_device)) + if self.librenms_id is None: @@ - current_librenms_id = self.librenms_api.get_librenms_id(sync_device) + current_librenms_id = coerce_librenms_id(self.librenms_api.get_librenms_id(sync_device))Based on learnings: explicit
is Nonesemantics should be used for storedlibrenms_idpresence checks instead of truthiness conversion.Also applies to: 527-530
🤖 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 329 - 331, The truthiness check at line 330 using `if not self.librenms_id:` does not properly handle malformed or edge-case CF values (strings, booleans) and can cause incorrect fetch behavior or repeated cache invalidation. Replace the truthiness gate `if not self.librenms_id:` with an explicit None check `if self.librenms_id is None:` at this location. Apply the same fix at the second occurrence mentioned around line 527-530 where `librenms_id` is also compared against cached fingerprint data. This ensures precise None semantics rather than relying on Python's truthiness conversion for stored identifier values.Source: Learnings
netbox_librenms_plugin/views/base/interfaces_view.py (1)
261-265:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the LibreNMS OOB ID out of the UI warning.
Line 255 already logs the OOB id for operators; echoing it in the user-facing toast exposes internal LibreNMS identifiers unnecessarily.
Proposed fix
messages.warning( request, - f"Interfaces refreshed, but OOB controller ports fetch failed (OOB id {oob['id']}); " + "Interfaces refreshed, but OOB controller ports fetch failed; " "showing host interfaces only. See server logs for details.", )🤖 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/interfaces_view.py` around lines 261 - 265, In the messages.warning call within the interfaces_view.py file, remove the internal LibreNMS OOB identifier from the user-facing warning message. The f-string currently includes (OOB id {oob['id']}) which exposes internal details to users; since this identifier is already logged at line 255 for operators, simplify the warning message to remove the OOB id reference while keeping the rest of the informative text intact.netbox_librenms_plugin/tests/test_modules_view.py (1)
51-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse an independent copy when asserting no mutation.
list(seed)shares the inner dict withseed, so a partial in-place mutation inside_merge_transceiver_data()can mutate both objects and still satisfyinventory == seed.Proposed test hardening
view = _make_view() view.librenms_id = 100 seed = [{"entPhysicalIndex": 1, "entPhysicalName": "Gi0/1"}] + + def _assert_malformed_payload(payload): + inventory_input = [dict(item) for item in seed] + view._librenms_api.get_device_transceivers.return_value = (True, payload) + inventory, error = view._merge_transceiver_data(inventory_input) + assert inventory == seed + assert inventory_input == seed + assert error and "malformed transceiver payload" in error # dict payload under success=True - view._librenms_api.get_device_transceivers.return_value = (True, {"unexpected": "dict"}) - inventory, error = view._merge_transceiver_data(list(seed)) - assert inventory == seed # untouched - assert error and "malformed transceiver payload" in error + _assert_malformed_payload({"unexpected": "dict"}) # list with a non-dict entry - view._librenms_api.get_device_transceivers.return_value = (True, [{"entity_physical_index": 2}, "bad"]) - inventory, error = view._merge_transceiver_data(list(seed)) - assert inventory == seed - assert error and "malformed transceiver payload" in error + _assert_malformed_payload([{"entity_physical_index": 2}, "bad"]) # empty NON-list payload ({}) is also malformed — it must NOT be treated as a successful # "no transceivers" response (which would cache a degraded snapshot). Regression for the # emptiness check running before the type check: {} is falsy, so the old order mislabeled it. - view._librenms_api.get_device_transceivers.return_value = (True, {}) - inventory, error = view._merge_transceiver_data(list(seed)) - assert inventory == seed - assert error and "malformed transceiver payload" in error + _assert_malformed_payload({})🤖 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_modules_view.py` around lines 51 - 69, The test uses list(seed) which creates a shallow copy, meaning inner dictionaries are still shared references with the original seed. If _merge_transceiver_data() mutates these dictionaries in-place, both inventory and seed will be modified together and the assertion inventory == seed will incorrectly pass. Replace all instances of list(seed) passed to _merge_transceiver_data() with a deep copy to ensure the original seed data remains completely independent and unmodified by the function being tested, which will properly catch any unintended in-place mutations.netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
459-478:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEvict the server-scoped IP cache when a fresh refresh fails.
_prepare_context(fetch_fresh=True)now returnsNonefor malformed LibreNMS payloads, but any previousip_addressescache entry remains intact. A later cached render can show or sync stale IP rows after the refresh was reported as failed.Proposed fix
if server_key is None: messages.error(request, "Selected LibreNMS server is no longer configured.") # Keep migrated-donor context (resolved from the POSTed key, since rebind failed) # so the template still suppresses the live sync form/button — a stale server_key # must not silently re-enable IP sync on a migrated donor. Mirrors cables_view. return render( request, self.partial_template_name, { "ip_sync": {"object": obj, "table": None, "cache_expiry": None, "server_key": None}, **build_migrated_context(obj, posted_server_key), }, ) + cache.delete(self.get_cache_key(obj, "ip_addresses", server_key)) context = self._prepare_context(request, obj, interface_name_field, fetch_fresh=True, server_key=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/ip_addresses_view.py` around lines 459 - 478, When _prepare_context with fetch_fresh=True returns None indicating a failed fetch, the server-scoped IP cache must be evicted to prevent stale data from persisting. In the block where context is None, add cache eviction logic for the IP addresses cache using the available server_key parameter before calling the error message and render method. This ensures that subsequent operations won't use cached IP rows that are now known to be stale after the fresh refresh failure.netbox_librenms_plugin/views/base/cables_view.py (2)
379-389:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve VC members from both local-port names.
Line 380 picks the chassis member only from
local_port, solocal_port_altcannot help when the displayed name is the one that fails VC-member resolution. Use the same candidate list for member selection before the widenedname__inlookup.🐛 Proposed fix
if hasattr(obj, "virtual_chassis") and obj.virtual_chassis: - chassis_member = get_virtual_chassis_member(obj, local_port) + chassis_member = None + for candidate in name_candidates: + chassis_member = get_virtual_chassis_member(obj, candidate) + if chassis_member: + break if chassis_member:🤖 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 379 - 389, The get_virtual_chassis_member call at line 380 only uses local_port as the lookup parameter, preventing local_port_alt from being considered during virtual chassis member resolution. Modify the code to attempt virtual chassis member resolution using the same name_candidates list that is used later for the widened name__in lookup, so that both local_port and local_port_alt can contribute to finding the correct chassis member before falling back to the broader interface name matching.
569-576:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t cache partial cable snapshots after fetch-side failures.
get_links_data()can return rows while setting_links_fetch_erroror_oob_links_fetch_failed; line 572 then stores that truncated host/OOB-only result as the authoritative cache, so later renders omit the failed side without any warning. Render the partial rows for this POST, but skip the cache write when the refresh is known partial.🐛 Proposed fix
cache_key = self.get_cache_key(cache_device, "links", server_key) if fetch_fresh: - cache.set( - cache_key, - {"links": links_data}, - timeout=self.librenms_api.cache_timeout, + partial_fetch_failed = getattr(self, "_oob_links_fetch_failed", False) or ( + getattr(self, "_links_fetch_error", None) + and getattr(self, "librenms_id", None) is not None ) + if not partial_fetch_failed: + cache.set( + cache_key, + {"links": links_data}, + timeout=self.librenms_api.cache_timeout, + )🤖 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 569 - 576, The cache write operation in the block around the cache.set() call is storing partial cable snapshot data even when fetch-side failures have occurred. Check if the links_data has `_links_fetch_error` or `_oob_links_fetch_failed` attributes set (indicating a partial refresh), and only execute the cache.set() block when both fetch operations succeeded. This prevents incomplete host/OOB-only results from being cached as authoritative data, which would cause later renders to silently omit the failed side without any warning.
🤖 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_views.py`:
- Around line 1292-1297: The test verifies the migrated context is merged into
the render, but it needs an additional assertion to confirm that live IP sync
state is disabled for the stale server. After the existing assertion for
ctx["migrated_to_marker"], add an assertion to verify that
ctx["ip_sync"]["server_key"] is None. This ensures that the stale key is not
exposed back to the live-sync controls and prevents regression in the IP sync
behavior when rendering stale servers.
In `@netbox_librenms_plugin/tests/test_interface_sync_content_template.py`:
- Around line 147-163: The test method
test_move_button_omits_server_key_hx_vals_when_marker_has_no_key currently only
verifies that empty server_key payloads are not present in the HTML, but does
not confirm that the hx-vals attribute itself is completely absent from the
move-to-winner element. Add a more targeted assertion after the existing
assertions that specifically checks that the move button element does not have
an hx-vals attribute at all when the marker has no server_key, making the test
more robust against cases where a non-empty hx-vals might still be rendered.
In `@netbox_librenms_plugin/utils.py`:
- Around line 1839-1844: Add validation for the marker's server_key field in
addition to the existing device_id validation. After extracting device_id from
the marker, also extract the server_key and validate it (the server_key must
exist and be a valid/non-empty value). Update the conditional check to return
None if either device_id validation fails OR if server_key validation fails,
ensuring that a malformed or copied marker with an incorrect server_key cannot
activate migrated mode for the wrong server. Only return the marker if both
device_id and server_key pass their respective validations.
---
Outside diff comments:
In `@netbox_librenms_plugin/tests/test_modules_view.py`:
- Around line 51-69: The test uses list(seed) which creates a shallow copy,
meaning inner dictionaries are still shared references with the original seed.
If _merge_transceiver_data() mutates these dictionaries in-place, both inventory
and seed will be modified together and the assertion inventory == seed will
incorrectly pass. Replace all instances of list(seed) passed to
_merge_transceiver_data() with a deep copy to ensure the original seed data
remains completely independent and unmodified by the function being tested,
which will properly catch any unintended in-place mutations.
In `@netbox_librenms_plugin/tests/test_vlan_sync.py`:
- Around line 613-625: In both test_missing_librenms_id_evicts_scoped_cache and
test_fetch_failure_evicts_scoped_cache, the current assertions using
mock_cache.delete.assert_any_call only verify that prod-scoped cache keys are
deleted but do not verify that default-scoped or other server-scoped keys are
NOT deleted. Add negative assertions after the existing assert_any_call
statements to ensure that mock_cache.delete was not called with default:vlans or
default:vlans:last keys, thereby enforcing server-key isolation and preventing
unintended cross-server cache bleed in both test methods.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 379-389: The get_virtual_chassis_member call at line 380 only uses
local_port as the lookup parameter, preventing local_port_alt from being
considered during virtual chassis member resolution. Modify the code to attempt
virtual chassis member resolution using the same name_candidates list that is
used later for the widened name__in lookup, so that both local_port and
local_port_alt can contribute to finding the correct chassis member before
falling back to the broader interface name matching.
- Around line 569-576: The cache write operation in the block around the
cache.set() call is storing partial cable snapshot data even when fetch-side
failures have occurred. Check if the links_data has `_links_fetch_error` or
`_oob_links_fetch_failed` attributes set (indicating a partial refresh), and
only execute the cache.set() block when both fetch operations succeeded. This
prevents incomplete host/OOB-only results from being cached as authoritative
data, which would cause later renders to silently omit the failed side without
any warning.
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 261-265: In the messages.warning call within the
interfaces_view.py file, remove the internal LibreNMS OOB identifier from the
user-facing warning message. The f-string currently includes (OOB id
{oob['id']}) which exposes internal details to users; since this identifier is
already logged at line 255 for operators, simplify the warning message to remove
the OOB id reference while keeping the rest of the informative text intact.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 459-478: When _prepare_context with fetch_fresh=True returns None
indicating a failed fetch, the server-scoped IP cache must be evicted to prevent
stale data from persisting. In the block where context is None, add cache
eviction logic for the IP addresses cache using the available server_key
parameter before calling the error message and render method. This ensures that
subsequent operations won't use cached IP rows that are now known to be stale
after the fresh refresh failure.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 329-331: The truthiness check at line 330 using `if not
self.librenms_id:` does not properly handle malformed or edge-case CF values
(strings, booleans) and can cause incorrect fetch behavior or repeated cache
invalidation. Replace the truthiness gate `if not self.librenms_id:` with an
explicit None check `if self.librenms_id is None:` at this location. Apply the
same fix at the second occurrence mentioned around line 527-530 where
`librenms_id` is also compared against cached fingerprint data. This ensures
precise None semantics rather than relying on Python's truthiness conversion for
stored identifier values.
🪄 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: a4b480d4-7bf2-49b9-8cf5-2bcd0093760e
📒 Files selected for processing (23)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_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_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_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/sync/migrate.py
marcinpsk
force-pushed
the
feat/device-merge
branch
from
June 20, 2026 22:43
4012721 to
81c16b7
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/views/base/interfaces_view.py (1)
180-180: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor: Inconsistent variable reference.
Line 180 uses
post_server_keywhile subsequent failure redirects (lines 186, 198) use_server_key. Since_server_key = post_server_keyon line 162, they're functionally identical, but using_server_keyconsistently would improve readability.Suggested fix
- return self._failure_redirect(request, obj, post_server_key) + return self._failure_redirect(request, obj, _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/interfaces_view.py` at line 180, In the _failure_redirect method call on line 180, the parameter is using post_server_key while the same method calls on lines 186 and 198 use _server_key instead. For consistency and readability, replace the post_server_key argument in the line 180 _failure_redirect call with _server_key to match the pattern used in the subsequent failure redirect calls throughout the method.netbox_librenms_plugin/views/base/modules_view.py (1)
429-430: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUnnecessary local variable assignment.
_server_keyis only used once immediately after assignment. Useserver_keydirectly.♻️ Suggested simplification
- _server_key = server_key - oob = get_librenms_oob(sync_device, server_key=_server_key) + oob = get_librenms_oob(sync_device, server_key=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 429 - 430, Remove the unnecessary local variable assignment of _server_key from server_key on line 429. In the get_librenms_oob function call on line 430, replace the _server_key parameter with server_key directly, since the intermediate variable is only used once and adds no value.
🤖 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/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 904-920: The issue is that when a radio is switched from "Use new"
back to "Keep current", the hidden override field is not being cleared because
the "Keep" radio option does not have the data-override-target attribute. When
the change event handler processes radios in the group (obtained via
querySelectorAll with the matching name), it only finds and clears the hidden
field if the checked radio has a data-override-target attribute. To fix this,
modify the logic so that when any radio in the group is checked with value
"keep", you find and clear the corresponding hidden override field(s) regardless
of whether the checked "Keep" radio itself has the data-override-target
attribute. You can accomplish this by either ensuring the "Keep" radio also has
the data-override-target attribute set, or by deriving the override field name
from the radio name pattern (converting _promote_ to override_) and clearing
that hidden field when the "keep" value is selected.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 2805-2833: The legacy librenms_id detection logic does not
explicitly reject boolean values before type coercion, allowing True/False to be
converted to 1/0 through int(). Add an explicit boolean guard immediately after
retrieving stored_id from existing_device.custom_field_data.get("librenms_id")
that checks if isinstance(stored_id, bool) and returns an error response if
true, or use the coerce_librenms_id() utility function to safely handle the
coercion. Apply this same fix in both the promote path (around line 2823-2824)
and the merge path at lines 3016-3031 to ensure both paths reject corrupt JSON
custom field data consistently.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Line 180: In the _failure_redirect method call on line 180, the parameter is
using post_server_key while the same method calls on lines 186 and 198 use
_server_key instead. For consistency and readability, replace the
post_server_key argument in the line 180 _failure_redirect call with _server_key
to match the pattern used in the subsequent failure redirect calls throughout
the method.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 429-430: Remove the unnecessary local variable assignment of
_server_key from server_key on line 429. In the get_librenms_oob function call
on line 430, replace the _server_key parameter with server_key directly, since
the intermediate variable is only used once and adds no value.
🪄 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: b376cfb7-7eac-4623-8f87-4c8af8d2f0b7
📒 Files selected for processing (23)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_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_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_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/sync/migrate.py
📜 Review details
🧰 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_interface_sync_content_template.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.py
**/urls.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use
<int:pk>(not<str:pk>) for numeric IDs in URL patterns to auto-validate and return 404 for non-integer values, eliminating URL-parameter taint that CodeQL flags
Files:
netbox_librenms_plugin/urls.py
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust export:validate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
**/views/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/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/vlan_table_view.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/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/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/htmx/device_validation_details.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments live in
templates/netbox_librenms_plugin/htmx/including:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
**/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
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/migrate.py
🧠 Learnings (35)
📚 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/views/__init__.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.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/views/__init__.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.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/views/__init__.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.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/views/__init__.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.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_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_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/__init__.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/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-08T19:01:30.947Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/views/base/librenms_sync_view.py:36-41
Timestamp: 2026-03-08T19:01:30.947Z
Learning: In netbox_librenms_plugin/views/base/librenms_sync_view.py, when checking for a stored librenms_id, use explicit None-check instead of a bare truthiness test. Specifically, in get_context_data(), prefer: self.librenms_api.get_librenms_id(librenms_sync_device) is not None rather than a boolean conversion. This ensures a stored value of 0 is treated as present (since 0 is falsy in Python) and avoids misclassifying it as missing. Do not replace with bool(...) or a plain if self.librenms_api.get_librenms_id(...) check. Rationale: differentiates between None (no mapping) and valid numeric ids (including 0).
Applied to files:
netbox_librenms_plugin/views/base/librenms_sync_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/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/sync/migrate.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/librenms_sync_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/vlan_table_view.py
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/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/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/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/htmx/device_validation_details.html
📚 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-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/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/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-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-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
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/migrate.py
🪛 ast-grep (0.43.0)
netbox_librenms_plugin/tests/test_migrate_views.py
[error] 395-395: Lack of sanitization of user data
Context: HttpResponse(status=403)
Note: [CWE-20].
(http-response-from-request)
[warning] 822-822: Do not make http calls without encryption
Context: "http://testserver/p/"
Note: [CWE-319].
(requests-http)
[warning] 823-823: Do not make http calls without encryption
Context: "http://testserver/p/"
Note: [CWE-319].
(requests-http)
[warning] 830-830: Do not make http calls without encryption
Context: "http://evil.example/p/"
Note: [CWE-319].
(requests-http)
netbox_librenms_plugin/views/base/ip_addresses_view.py
[error] 480-484: Avoid HTML built in strings
Context: render(
request,
self.partial_template_name,
{"ip_sync": context, **build_migrated_context(obj, server_key)},
)
Note: [CWE-79].
(html-string-from-parameters)
netbox_librenms_plugin/views/base/modules_view.py
[error] 325-325: Filename coming from the request
Context: request.POST.get("server_key")
Note: [CWE-22].
(open-filename-from-request)
netbox_librenms_plugin/views/base/cables_view.py
[error] 672-676: Avoid HTML built in strings
Context: render(
request,
self.partial_template_name,
{"cable_sync": context, **build_migrated_context(obj, server_key)},
)
Note: [CWE-79].
(html-string-from-parameters)
netbox_librenms_plugin/tests/test_coverage_actions.py
[error] 4772-4772: Lack of sanitization of user data
Context: HttpResponse("Forbidden", status=403)
Note: [CWE-20].
(http-response-from-request)
[error] 4858-4858: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 4888-4888: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 4936-4936: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 4973-4973: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
[error] 5491-5491: Lack of sanitization of user data
Context: HttpResponse("row")
Note: [CWE-20].
(http-response-from-request)
netbox_librenms_plugin/views/base/vlan_table_view.py
[error] 56-56: Avoid HTML built in strings
Context: render(request, self.partial_template_name, context)
Note: [CWE-79].
(html-string-from-parameters)
[error] 54-54: Filename coming from the request
Context: request.POST.get("server_key")
Note: [CWE-22].
(open-filename-from-request)
[error] 82-82: Avoid HTML built in strings
Context: render(request, self.partial_template_name, context)
Note: [CWE-79].
(html-string-from-parameters)
[error] 87-87: Avoid HTML built in strings
Context: render(request, self.partial_template_name, context)
Note: [CWE-79].
(html-string-from-parameters)
netbox_librenms_plugin/views/imports/actions.py
[info] 2930-2930: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"validationRefresh": {"deviceId": device_id}})
Note: Security best practice.
(use-jsonify)
[info] 2939-2939: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"validationRefresh": {"deviceId": device_id}})
Note: Security best practice.
(use-jsonify)
[error] 2738-2738: Filename coming from the request
Context: request.POST.get("existing_device_id")
Note: [CWE-22].
(open-filename-from-request)
[error] 2742-2742: Filename coming from the request
Context: request.POST.get("server_key")
Note: [CWE-22].
(open-filename-from-request)
[error] 2761-2761: Filename coming from the request
Context: request.POST.get("override_name")
Note: [CWE-22].
(open-filename-from-request)
[error] 2762-2762: Filename coming from the request
Context: request.POST.get("override_device_type_id")
Note: [CWE-22].
(open-filename-from-request)
[error] 2763-2763: Filename coming from the request
Context: request.POST.get("override_platform_id")
Note: [CWE-22].
(open-filename-from-request)
[error] 2969-2969: Filename coming from the request
Context: request.POST.get("server_key")
Note: [CWE-22].
(open-filename-from-request)
[error] 2976-2976: Filename coming from the request
Context: request.POST.get("winner_pk")
Note: [CWE-22].
(open-filename-from-request)
netbox_librenms_plugin/views/sync/migrate.py
[error] 188-188: Lack of sanitization of user data
Context: HttpResponse(status=status, headers={"HX-Refresh": "true"})
Note: [CWE-20].
(http-response-from-request)
[error] 323-323: Lack of sanitization of user data
Context: HttpResponse(toast_html, content_type="text/html")
Note: [CWE-20].
(http-response-from-request)
[error] 98-98: Filename coming from the request
Context: request.POST.get("server_key")
Note: [CWE-22].
(open-filename-from-request)
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
[error] 719-719: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 720-720: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 801-801: Duplicate of attribute name [ new_name ] was found.
(attr-no-duplication)
[error] 925-925: Tag must be paired, no start tag: [ ]
(tag-pair)
marcinpsk
force-pushed
the
feat/device-merge
branch
from
June 21, 2026 05:49
81c16b7 to
ce97477
Compare
marcinpsk
force-pushed
the
feat/device-merge
branch
from
July 31, 2026 10:51
41dfb01 to
8de264c
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.github/workflows/codeql.yml:
- Line 72: Update both github/codeql-action uses steps in the workflow,
including init and the symbol at the second referenced line, to pin v4.37.3 by
its immutable release commit SHA instead of the mutable tag. Preserve the
v4.37.3 version in a trailing comment on each pinned action reference.
In @.github/workflows/lint-format.yaml:
- Line 21: Update the actions/checkout step in the lint workflow to set
persist-credentials to false, ensuring the checkout token is unavailable to
later tooling such as ruff.
In @.github/workflows/publish-pypi.yaml:
- Around line 24-26: Update the checkout step in the publish workflow to set
persist-credentials to false, while preserving the existing actions/checkout
version and subsequent setup-python step.
In `@netbox_librenms_plugin/librenms_api.py`:
- Around line 803-813: Update the no-IP detection in the 404 handling around the
message extraction to normalize the returned message and use a case-insensitive
substring check for the device’s no-IP wording instead of exact equality. Keep
unrelated 404 responses failing closed with the existing `(False, message or
str(e))` behavior.
In `@netbox_librenms_plugin/utils.py`:
- Around line 2157-2172: Extend the merge conflict guard before the host-ID
transfer branches to also reject cases where winner_id and donor_id differ and
donor_oob_has_valid_id is true, even when winner_oob is None. Update the
ValueError message to describe the donor’s OOB link claiming the available slot,
preserving the existing “unlink one side first” guidance; do not allow the later
inheritance path to drop donor_id.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 321-325: Update UpdateModuleInterfaceView.post() to invoke
_module_interface_update_message() for every "bound" result, including unchanged
results with adopted_count equal to zero, so the no-op message is reachable.
Preserve the existing message behavior for changed and adopted interfaces. Add
or update the test at
netbox_librenms_plugin/tests/test_sync_modules.py:5810-5816 to exercise post()
and assert the expected no-op message is emitted; the view change is required at
netbox_librenms_plugin/views/sync/modules.py:321-325.
🪄 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 Plus
Run ID: 5a7bc905-13e8-43bb-abcb-2d7dd3924fec
📒 Files selected for processing (61)
.github/workflows/codeql.yml.github/workflows/lint-format.yaml.github/workflows/mkdocs.yaml.github/workflows/publish-pypi.yaml.github/workflows/test.yamlnetbox_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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_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/_add_as_oob_form.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/_html_helpers.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_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_view.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_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_multiserver_get_cache_scoping.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_page_server_key_forms.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_utils_shared_helpers.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/sync/device_fields.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
| # Initializes the CodeQL tools for scanning. | ||
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@v4 | ||
| uses: github/codeql-action/init@v4.37.3 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Pin github/codeql-action to a commit SHA.
Lines 72 and 101 use the mutable tag v4.37.3. .github/workflows/lint-format.yaml and .github/workflows/mkdocs.yaml pin actions by commit SHA with a version comment. zizmor reports unpinned-uses for both lines under a blanket policy. Pin both steps to the SHA of the v4.37.3 release and keep the version in a trailing comment.
🔒 Proposed pinning change
- uses: github/codeql-action/init@v4.37.3
+ uses: github/codeql-action/init@<commit-sha> # v4.37.3- uses: github/codeql-action/analyze@v4.37.3
+ uses: github/codeql-action/analyze@<commit-sha> # v4.37.3Also applies to: 101-101
🧰 Tools
🪛 zizmor (1.28.0)
[error] 72-72: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/codeql.yml at line 72, Update both github/codeql-action
uses steps in the workflow, including init and the symbol at the second
referenced line, to pin v4.37.3 by its immutable release commit SHA instead of
the mutable tag. Preserve the v4.37.3 version in a trailing comment on each
pinned action reference.
Source: Linters/SAST tools
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| persist-credentials: false | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check persist-credentials usage across workflows and confirm the removal in git history.
set -euo pipefail
fd -e yml -e yaml . .github/workflows --exec sh -c 'echo "== $1"; rg -n -C 4 "actions/checkout|persist-credentials" "$1"' sh {}
echo "== history for lint-format.yaml"
git log --oneline -5 -- .github/workflows/lint-format.yamlRepository: marcinpsk/netbox-librenms-plugin
Length of output: 3355
🌐 Web query:
actions/checkout v7 persist-credentials default true writes GITHUB_TOKEN in .git/config
💡 Result:
In actions/checkout v7, the default value for persist-credentials remains true [1][2]. However, the behavior regarding where credentials are stored has changed significantly in recent major versions [1][3]. Starting in version v6.0.0, the actions/checkout action was updated to store credentials in a separate file under the runner's temporary directory ($RUNNER_TEMP) rather than writing them directly into the local.git/config file [3]. This change was implemented to improve security, specifically to prevent credentials from being inadvertently exposed (for example, if files were copied or if an artifact was created that included the.git directory) [4]. When persist-credentials is set to true (the default), the action still configures Git to use these credentials for authenticated Git commands (such as fetch or push) by referencing the file in the temporary directory [1][3]. The credentials are then removed during the post-job cleanup phase [3]. While the mechanism for storage has moved out of the local.git/config file to enhance security, the functional effect—allowing your workflow scripts to perform authenticated Git operations using the repository's access token—remains enabled by default [1][3].
Citations:
- 1: actions/checkout@v4.2.2...v7
- 2: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 3: https://github.com/actions/checkout/tree/v6.0.0
- 4: [security] Escalate concerning default
persist-credentials=trueplease actions/checkout#2312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lint-format workflow relevant sections =="
cat -n .github/workflows/lint-format.yaml | sed -n '1,120p'
echo "== lint-format diff in latest commits =="
git diff -- .github/workflows/lint-format.yaml
echo "== inspect persisted credential handling in checked out v7 action metadata if available upstream =="
python3 - <<'PY'
import urllib.request
url = "https://raw.githubusercontent.com/actions/checkout/3d3c42e5aac5ba805825da76410c181273ba90b1/action.yml"
try:
text = urllib.request.urlopen(url, timeout=20).read().decode()
for i, line in enumerate(text.splitlines(), 1):
if "persist-credentials" in line:
print(f"{i}: {line}")
except Exception as e:
print(f"FETCH_ERROR: {e}")
PYRepository: marcinpsk/netbox-librenms-plugin
Length of output: 2151
🌐 Web query:
GitHub actions/checkout v7 persist-credentials default true official documentation
💡 Result:
In GitHub Actions actions/checkout version 7, the persist-credentials input continues to have a default value of true [1][2]. While there has been community discussion and proposed pull requests to change this default to false to enhance security [3], the official documentation and the action.yml configuration file for version 7 maintain the original behavior [1][2]. When persist-credentials is set to true (the default), the action configures the GITHUB_TOKEN or SSH key in the local git configuration, enabling subsequent steps in your workflow to execute authenticated git commands [1][4]. This credential is automatically removed during the post-job cleanup phase [1][5].
Citations:
- 1: https://github.com/actions/checkout/?tab=readme-ov-file
- 2: https://github.com/actions/checkout/blob/72f2cec99f417b1a1c5e2e88945068983b7965f9/action.yml
- 3: Change the default value of persist-credentials to false actions/checkout#1687
- 4: https://github.com/marketplace/actions/checkout
- 5: https://github.com/actions/checkout
Restore persist-credentials: false on the checkout step.
actions/checkout defaults to persisting credentials, and this lint job later runs installed third-party tooling. Set persist-credentials: false so the checkout token is not available to ruff.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 20-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/lint-format.yaml at line 21, Update the actions/checkout
step in the lint workflow to set persist-credentials to false, ensuring the
checkout token is unavailable to later tooling such as ruff.
This was referenced Aug 13, 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #113 — review only the delta over
pr/oob-sync.Device merge / move-to-winner migration. When two NetBox devices represent one physical box (e.g. a host and its separately-imported OOB controller), pick a winner and move the donor's interfaces, IP addresses and cables onto it, then mark the donor migrated. A migrated donor's sync controls are suppressed so it isn't re-synced.
Motivation / Problem
Feature. Consolidate duplicate NetBox devices found during import without losing interfaces/IPs/cables.
Scope of Change
How Was This Tested?
Risk Assessment
Moves FKs between existing devices; gated behind explicit user action. No automatic merges.
Backwards Compatibility
Summary by CodeRabbit
New Features
Bug Fixes