fix: multi-server hardening — code correctness, security fixes, and expanded test coverage - #21
Closed
marcinpsk wants to merge 49 commits into
Closed
fix: multi-server hardening — code correctness, security fixes, and expanded test coverage#21marcinpsk wants to merge 49 commits into
marcinpsk wants to merge 49 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds multi‑server LibreNMS support: per‑server librenms_id mapping and migration, server_key‑scoped cache keys and flows, threading naming preferences through import/validation, server‑aware sync views/UI, VC naming improvements, many tests and a local mock LibreNMS test server. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/UI
participant View as Sync View
participant Cache as Cache/Redis
participant API as LibreNMSAPI
participant DB as NetBox DB
Client->>View: GET/POST (includes server_key)
View->>View: extract server_key, build cache key
View->>Cache: get_cache_key(obj, data_key, server_key)
alt Cache Hit
Cache-->>View: cached data
View->>Client: render using cached data
else Cache Miss
View->>API: fetch data (server_key context)
API-->>View: data
View->>Cache: store data with server-scoped key
View->>DB: update object/custom_fields (set_librenms_device_id(server_key))
View->>Client: response (redirect preserves server_key)
end
sequenceDiagram
participant Import as Import Job
participant Validation as validate_device_for_import
participant Cache as Cache/Redis
participant API as LibreNMSAPI
participant DB as NetBox DB
Import->>Validation: validate_device_for_import(libre_device, server_key, use_sysname, strip_domain)
Validation->>Cache: get_validated_device_cache_key(server_key,...,use_sysname,strip_domain)
alt Cache Hit
Cache-->>Validation: cached validation
Validation-->>Import: return cached result
else Cache Miss
Validation->>API: fetch device details (server_key)
Validation->>DB: lookup device/type, VC checks
Validation->>Validation: apply naming prefs & chassis fallback
Validation->>Cache: store validation result keyed by server_key+prefs
Validation-->>Import: return result
end
Import->>DB: create/update device/VM with set_librenms_device_id(server_key)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
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/tables/cables.py (1)
122-137:⚠️ Potential issue | 🔴 CriticalEscape VC member names before marking the option list safe.
member.nameis interpolated into raw<option>HTML and then passed throughmark_safe(). A crafted chassis member name would render as executable HTML/JS in this table.🔒 Proposed fix
-from django.utils.html import format_html +from django.utils.html import format_html, format_html_join from django.utils.safestring import mark_safe @@ - options = [ - f'<option value="{member.id}"{" selected" if member.id == selected_member_id else ""}>{member.name}</option>' - for member in members - ] + options = format_html_join( + "", + '<option value="{}"{}>{}</option>', + ( + ( + member.id, + mark_safe(" selected") if member.id == selected_member_id else "", + member.name, + ) + for member in members + ), + ) return format_html( '<select name="device_selection_{0}" id="device_selection_{0}" class="form-select" data-interface="{0}" data-row-id="{0}">{1}</select>', port_id, - mark_safe("".join(options)), + options, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/cables.py` around lines 122 - 137, The options list in render_device_selection interpolates member.name directly into HTML and then mark_safe() is applied, allowing XSS; fix by escaping member.name before composing the option string (use Django's escape from django.utils.html or conditional_escape) so each option uses escaped member names (e.g., escape(member.name)) and then join/mark_safe the resulting option strings; update render_device_selection to build options with escaped member.name and keep using mark_safe for the final concatenated HTML.netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (2)
615-631:⚠️ Potential issue | 🟠 MajorDon't close the modal before the override write succeeds.
The request started on Line 615 is still fire-and-forget. On any 4xx/5xx/network failure, the current page looks updated but the shared override cache stays stale, so the change disappears on the next tab/page load while the modal closes as if the bulk apply succeeded.
🔧 Keep the modal open on failure
- saveBtn.addEventListener('click', function () { + saveBtn.addEventListener('click', async function () { @@ - fetch('/plugins/librenms_plugin/save-vlan-group-overrides/', { + try { + const response = await fetch('/plugins/librenms_plugin/save-vlan-group-overrides/', { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value }, body: JSON.stringify({ device_id: deviceId, vid_group_map: vidGroupMap, server_key: document.getElementById('current-server-key')?.value || null }) - }).then(response => { - if (!response.ok) { - console.error('Failed to persist VLAN group overrides: HTTP', response.status); - } - }).catch(error => { + }); + if (!response.ok) { + throw new Error((await response.text()) || `HTTP ${response.status}`); + } + } catch (error) { console.error('Failed to persist VLAN group overrides:', error.message); - }); + return; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 615 - 631, The fetch to '/plugins/librenms_plugin/save-vlan-group-overrides/' is currently fire-and-forget so the modal is closed regardless of success; change the flow to only close the modal after a successful response.ok. Modify the promise handling around the fetch that sends {device_id: deviceId, vid_group_map: vidGroupMap, server_key: ...} so that you check response.ok, parse any error body if available, and on success proceed to close the modal (the same UI-close call currently executed elsewhere), but on non-ok or network error keep the modal open and surface a visible error message (use response.status/text or error.message). Ensure you reference the existing fetch call and the variables deviceId, vidGroupMap and document.getElementById('current-server-key') when making the change.
789-827:⚠️ Potential issue | 🟠 MajorGuard the interface-name radio lookup before the fetch.
The HTTP error handling added here is good, but Line 799 still assumes a checked
input[name="interface_name_field"]exists. If that control is missing or temporarily unchecked after an HTMX swap, this throws before the verify request is sent.🛡️ Null-check the selected radio first
function handleInterfaceChange(select, value) { + const interfaceNameField = + document.querySelector('input[name="interface_name_field"]:checked')?.value; + if (!interfaceNameField) { + console.error('Missing interface_name_field selection'); + return; + } + fetch('/plugins/librenms_plugin/verify-interface/', { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value }, body: JSON.stringify({ device_id: value, interface_name: select.dataset.interface, - interface_name_field: document.querySelector('input[name="interface_name_field"]:checked').value, + interface_name_field: interfaceNameField, server_key: document.getElementById('current-server-key')?.value || null }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 789 - 827, In handleInterfaceChange, guard the radio lookup used for interface_name_field before creating the fetch body: locate the selector 'input[name="interface_name_field"]:checked' into a variable (e.g. selectedRadio) and use selectedRadio ? selectedRadio.value : null (or a sensible default) when building the JSON body instead of accessing .value unconditionally; this prevents a runtime error if the checked radio is missing (ensure you keep use of device_id, interface_name, server_key, and the existing fetch/error handling).netbox_librenms_plugin/views/imports/list.py (1)
289-320: 🧹 Nitpick | 🔵 TrivialConsider extracting naming preference resolution into a helper.
The preference resolution logic (lines 291-307) duplicates the pattern from lines 144-161 and appears again in
_get_import_queryset(lines 443-460). This could be simplified with a shared helper method.♻️ Optional: Extract preference resolution helper
def _resolve_naming_preferences(self, request): """Resolve use_sysname and strip_domain from user prefs or settings defaults.""" try: settings_obj = LibreNMSSettings.objects.first() except Exception: logger.exception("Failed to load LibreNMSSettings for naming preferences") settings_obj = None use_sysname_pref = get_user_pref(request, "plugins.netbox_librenms_plugin.use_sysname") strip_domain_pref = get_user_pref(request, "plugins.netbox_librenms_plugin.strip_domain") use_sysname = ( use_sysname_pref if use_sysname_pref is not None else (getattr(settings_obj, "use_sysname_default", True) if settings_obj else True) ) strip_domain = ( strip_domain_pref if strip_domain_pref is not None else (getattr(settings_obj, "strip_domain_default", False) if settings_obj else False) ) return use_sysname, strip_domain🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/imports/list.py` around lines 289 - 320, The naming preference resolution logic duplicated in the view (the block that reads LibreNMSSettings and derives _use_sysname and _strip_domain before calling FilterDevicesJob.enqueue) should be extracted into a reusable helper method (e.g., _resolve_naming_preferences(self, request)) and then used wherever that logic appears (the current block before FilterDevicesJob.enqueue and the similar code in _get_import_queryset); implement the helper to try loading LibreNMSSettings.objects.first() with exception handling, read get_user_pref for "plugins.netbox_librenms_plugin.use_sysname" and "plugins.netbox_librenms_plugin.strip_domain", compute use_sysname and strip_domain with the same fallback semantics (use user pref if not None, else settings defaults or True/False), and replace the inline code with a call to _resolve_naming_preferences(request) to return use_sysname and strip_domain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Line 17: Update the check-yaml exclude regex value for precision: replace the
current pattern used in the YAML entry "exclude: mkdocs\.yml$" with a pattern
that only matches a basename, e.g. "(^|/)mkdocs\.yml$", so only files actually
named "mkdocs.yml" are excluded (not "foo_mkdocs.yml" or similar); keep the
change in the same "exclude:" setting.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 643-649: The metadata payload written alongside the cache key must
include the naming-mode flags so different naming modes are distinguishable by
get_active_cached_searches(); update the code paths that build the metadata body
(the same area that calls get_cache_metadata_key using server_key, filters,
vc_detection_enabled, use_sysname, strip_domain) to also set use_sysname and
strip_domain in the metadata dict/object saved to storage, and do the same for
the second occurrence of this logic (the block around the later call that
mirrors lines 661-667); ensure the metadata keys exactly match the boolean flags
(use_sysname, strip_domain) so get_active_cached_searches() can list/restore
searches correctly.
In `@netbox_librenms_plugin/import_utils/cache.py`:
- Around line 33-36: get_cache_metadata_key treats None as “unset” but the
hash-based cache-key helpers still include None entries, causing inconsistent
keys; update the hash-based helpers to canonicalize filters the same way
(exclude entries where value is None) and/or delegate to
get_validated_device_cache_key so both synchronous and background paths generate
identical validated-device/search keys; change the helper(s) referenced in this
diff and the other occurrences (the two other hash-based key generators called
elsewhere) to filter out v is None when building filter_parts or to call
get_validated_device_cache_key(filter_dict, ...) to produce the final key.
In `@netbox_librenms_plugin/import_utils/virtual_chassis.py`:
- Around line 243-252: The loader returns raw settings.vc_member_name_pattern
which can be None or non-string and later causes pattern.format(...) to raise;
update _load_vc_member_name_pattern to validate the retrieved value from
LibreNMSSettings (in the settings = LibreNMSSettings.objects... block), ensuring
it is a non-empty string (e.g., isinstance check and/or str() + strip()) and
otherwise return the safe default "-M{position}", and keep the existing
exception handling path to also return the default.
In `@netbox_librenms_plugin/import_utils/vm_operations.py`:
- Around line 58-60: The code currently does librenms_device_id =
int(libre_device["device_id"]) which will coerce booleans (True/False) to 1/0;
update the validation in the VM creation path (the code that sets
librenms_device_id before creating the VM) to explicitly reject boolean values
first (e.g., check isinstance(libre_device["device_id"], bool) or compare type
to bool) and raise/return an error if it's a bool, then safely coerce valid
numeric/string IDs with int(...) afterwards; ensure this check is applied where
librenms_device_id is assigned so malformed payloads fail fast.
In `@netbox_librenms_plugin/tables/device_status.py`:
- Around line 474-500: render_actions builds the buttons list but leaves some
icon-only controls unlabeled (the NetBox link and the ready-state details
button); update render_actions to add an accessible name to those elements by
including an aria-label attribute (reuse the existing btn_title/aria_attr where
appropriate or supply explicit labels like "View in NetBox" and "View details")
for the NetBox anchor and the details button generated with details_url, and
ensure the aria-label is present when the button/icon has no visible text so
screen readers will announce them.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.html`:
- Line 10: The hidden input for server_key in the IP sync template is missing
the id the JS expects; update the existing hidden input that renders when
ip_sync.server_key is present (the one with name="server_key" and value="{{
ip_sync.server_key }}") to also include id="current-server-key" so the frontend
JS (used by SingleIPAddressVerifyView) can read the correct server_key instead
of falling back to "default".
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Line 132: This cell currently outputs raw libre_device.sysName or
libre_device.hostname; update it to render the resolved LibreNMS display name
that validation/sync actually use (e.g. the value computed in the view/context
such as libre_device.resolved_name or display_name) instead of sysName/hostname.
Locate where the resolved name is computed for validation/sync, expose that
property on the libre_device context object if not already present, and replace
the template expression (currently
libre_device.sysName|default:libre_device.hostname) with the resolved name
variable so the modal shows the exact name used by validation and sync.
In `@netbox_librenms_plugin/tests/test_utils.py`:
- Around line 359-433: The two test classes duplicate the same contract for
_safe_disabled; replace them with a single parametrized pytest that defines a
table of (input, expected) cases (covering all cases from
TestSafeDisabledBulkImport) and then runs each case against both targets
(import_utils.bulk_import._safe_disabled and
import_utils.filters._safe_disabled). Implement this by creating a param list
like [(True,1), (False,0), ("true",1), ("TRUE",1), ("yes",1), ("on",1),
("false",0), ("no",0), ("off",0), (1,1), (0,0), (None,0), ({},0) for the
missing-key case], use `@pytest.mark.parametrize` over the inputs/expected, import
the two functions by name and assert both return the expected value; keep test
names in one class or module and remove the duplicated
TestSafeDisabledFilters/TestSafeDisabledBulkImport classes.
In `@netbox_librenms_plugin/utils.py`:
- Around line 505-507: The code currently mutates
obj.custom_field_data["librenms_id"] in both normalization branches even when
auto_save is False; change the logic to compute/normalize int_id into a local
variable and only assign to obj.custom_field_data (and call obj.save()) when
auto_save is True—leave obj.custom_field_data untouched when auto_save is False;
apply the same change to the other branch around the 519-523 area so both
normalization paths only mutate the instance and call obj.save() when auto_save
is True.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 107-112: The _prefetch_netbox_data() path is read-only but calls
get_librenms_device_id() which defaults to auto_save=True and can trigger
interface.save(); update the call inside the loop that builds
interfaces_by_librenms_id to pass auto_save=False to
get_librenms_device_id(interface, server_key) (locate the loop iterating
all_interfaces and the interfaces_by_librenms_id assignment) so
loading/refreshing the IP sync table does not persist changes.
In `@netbox_librenms_plugin/views/base/librenms_sync_view.py`:
- Around line 167-206: The loop that builds the server mapping (using cf_value
and iterating for sk, did in cf_value.items()) must skip invalid LibreNMS IDs
(bool, None, arbitrary strings) so only usable mappings are emitted; add
validation for did (e.g., accept integers or digit-only strings, converting
digit-strings to int) and if did fails validation continue the loop without
appending to result; update the construction that uses device_id/device_url to
rely on the validated/converted integer ID so downstream helpers that expect
numeric IDs (and the all_server_mappings helper) no longer receive bogus
entries.
In `@netbox_librenms_plugin/views/object_sync/devices.py`:
- Around line 83-100: SingleInterfaceVerifyView.post() instantiates the
interface table without passing server_key, so format_interface_data() ends up
using the default server when calling get_librenms_device_id; update the table
construction in SingleInterfaceVerifyView.post() (where table_class is
instantiated) to include server_key=self.librenms_api.server_key (matching
get_table()’s behavior) so that format_interface_data() sees the correct
self.server_key and queries the proper per-server namespace.
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 127-129: The current branch collapses create_cable() failures into
an "invalid" status; change the return to emit a distinct status (e.g., "error")
when create_cable(local_interface, remote_interface, self.request) returns falsy
so callers can distinguish creation errors from genuinely missing links, and
ensure callers process_interface_sync() and display_sync_results() handle the
new "error" status by adding an error bucket/message; update references to
create_cable, process_interface_sync, and display_sync_results to
accept/propagate the "error" status and include the display_name and any error
context in the error bucket.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 562-565: The view currently accepts object_type values like
"virtualmachine" / "vm" and then unconditionally accesses obj.serial (and
compares it later), which will raise AttributeError on NetBox versions where
VirtualMachine.serial does not exist; update the code that handles object_type
(the branch using object_type/"virtualmachine" and the subsequent obj.serial
access) to either use a defensive getter (use getattr(obj, "serial", None)
wherever obj.serial is read/compared) to match the pattern used in actions.py,
or explicitly reject VM conversion when serial is unsupported and return a clear
error; ensure all references to obj.serial in this view use the guarded form or
trigger the rejection path.
In `@pyproject.toml`:
- Around line 69-70: The pyproject.toml contains an empty dependency group
declaration ([dependency-groups] with dev = []), which is a no-op; either remove
this section or populate the dev group and update CI/build invocation. To fix:
either delete the [dependency-groups] entry entirely if you will continue using
requirements_dev.txt, or migrate the dev packages (e.g., pytest, pytest-django)
into the dev group in pyproject.toml and update CI/build scripts to install with
the --group dev flag (and remove or stop sourcing requirements_dev.txt). Ensure
you update references in CI/config to use --group dev if you choose migration.
---
Outside diff comments:
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 615-631: The fetch to
'/plugins/librenms_plugin/save-vlan-group-overrides/' is currently
fire-and-forget so the modal is closed regardless of success; change the flow to
only close the modal after a successful response.ok. Modify the promise handling
around the fetch that sends {device_id: deviceId, vid_group_map: vidGroupMap,
server_key: ...} so that you check response.ok, parse any error body if
available, and on success proceed to close the modal (the same UI-close call
currently executed elsewhere), but on non-ok or network error keep the modal
open and surface a visible error message (use response.status/text or
error.message). Ensure you reference the existing fetch call and the variables
deviceId, vidGroupMap and document.getElementById('current-server-key') when
making the change.
- Around line 789-827: In handleInterfaceChange, guard the radio lookup used for
interface_name_field before creating the fetch body: locate the selector
'input[name="interface_name_field"]:checked' into a variable (e.g.
selectedRadio) and use selectedRadio ? selectedRadio.value : null (or a sensible
default) when building the JSON body instead of accessing .value
unconditionally; this prevents a runtime error if the checked radio is missing
(ensure you keep use of device_id, interface_name, server_key, and the existing
fetch/error handling).
In `@netbox_librenms_plugin/tables/cables.py`:
- Around line 122-137: The options list in render_device_selection interpolates
member.name directly into HTML and then mark_safe() is applied, allowing XSS;
fix by escaping member.name before composing the option string (use Django's
escape from django.utils.html or conditional_escape) so each option uses escaped
member names (e.g., escape(member.name)) and then join/mark_safe the resulting
option strings; update render_device_selection to build options with escaped
member.name and keep using mark_safe for the final concatenated HTML.
In `@netbox_librenms_plugin/views/imports/list.py`:
- Around line 289-320: The naming preference resolution logic duplicated in the
view (the block that reads LibreNMSSettings and derives _use_sysname and
_strip_domain before calling FilterDevicesJob.enqueue) should be extracted into
a reusable helper method (e.g., _resolve_naming_preferences(self, request)) and
then used wherever that logic appears (the current block before
FilterDevicesJob.enqueue and the similar code in _get_import_queryset);
implement the helper to try loading LibreNMSSettings.objects.first() with
exception handling, read get_user_pref for
"plugins.netbox_librenms_plugin.use_sysname" and
"plugins.netbox_librenms_plugin.strip_domain", compute use_sysname and
strip_domain with the same fallback semantics (use user pref if not None, else
settings defaults or True/False), and replace the inline code with a call to
_resolve_naming_preferences(request) to return use_sysname and strip_domain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6966375-bdc8-4746-b05b-0ddb2e46c718
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.gitignore.pre-commit-config.yamldocs/development/testing.mdnetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/filters.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.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/_vlan_sync_content.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/mock_librenms_server.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_interfaces.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vm_operations.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/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/object_sync/vms.pynetbox_librenms_plugin/views/sync/cables.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/vlans.pypyproject.toml
💤 Files with no reviewable changes (1)
- .gitignore
| - id: end-of-file-fixer | ||
| - id: check-yaml | ||
| exclude: ^mkdocs\.yml$ | ||
| exclude: mkdocs\.yml$ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files '*.yml' '*.yaml' | python - <<'PY'
import re
import sys
paths = [line.strip() for line in sys.stdin if line.strip()]
current = re.compile(r'mkdocs\.yml$')
anchored = re.compile(r'(^|/)mkdocs\.yml$')
print("Current pattern matches:")
for path in paths:
if current.search(path):
print(f" {path}")
print("\nBasename-anchored pattern matches:")
for path in paths:
if anchored.search(path):
print(f" {path}")
PYRepository: marcinpsk/netbox-librenms-plugin
Length of output: 138
🏁 Script executed:
cat -n .pre-commit-config.yaml | head -30Repository: marcinpsk/netbox-librenms-plugin
Length of output: 770
🏁 Script executed:
rg "mkdocs" --max-count 20Repository: marcinpsk/netbox-librenms-plugin
Length of output: 104
🏁 Script executed:
git ls-files | grep -E "(mkdocs|README)" | head -20Repository: marcinpsk/netbox-librenms-plugin
Length of output: 218
Tighten the check-yaml exclude regex for precision.
The pattern mkdocs\.yml$ matches any file path ending with mkdocs.yml, including unrelated files like foo_mkdocs.yml or nested paths like docs/mkdocs.yml. To exclude only files actually named mkdocs.yml, anchor to the basename.
Proposed fix
- exclude: mkdocs\.yml$
+ exclude: (^|/)mkdocs\.yml$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| exclude: mkdocs\.yml$ | |
| exclude: (^|/)mkdocs\.yml$ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml at line 17, Update the check-yaml exclude regex
value for precision: replace the current pattern used in the YAML entry
"exclude: mkdocs\.yml$" with a pattern that only matches a basename, e.g.
"(^|/)mkdocs\.yml$", so only files actually named "mkdocs.yml" are excluded (not
"foo_mkdocs.yml" or similar); keep the change in the same "exclude:" setting.
Comment on lines
643
to
649
| cache_metadata_key = get_cache_metadata_key( | ||
| server_key=api.server_key, filters=filters, vc_enabled=vc_detection_enabled | ||
| server_key=api.server_key, | ||
| filters=filters, | ||
| vc_enabled=vc_detection_enabled, | ||
| use_sysname=use_sysname, | ||
| strip_domain=strip_domain, | ||
| ) |
There was a problem hiding this comment.
Persist use_sysname and strip_domain in the metadata body too.
The key now distinguishes these naming modes, but the stored metadata does not. get_active_cached_searches() only has the metadata payload to list/restore saved searches, so two cached searches with identical filters but different naming modes become indistinguishable there.
Suggested fix
cache_metadata = {
"cached_at": datetime.now(timezone.utc).isoformat(),
"cache_timeout": api.cache_timeout,
"filters": filters,
"vc_enabled": vc_detection_enabled,
+ "use_sysname": use_sysname,
+ "strip_domain": strip_domain,
"device_count": len(validated_devices),
}Based on learnings: "Use get_active_cached_searches() to manage multi-search cache, allowing users to run and switch between searches."
Also applies to: 661-667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/import_utils/bulk_import.py` around lines 643 - 649,
The metadata payload written alongside the cache key must include the
naming-mode flags so different naming modes are distinguishable by
get_active_cached_searches(); update the code paths that build the metadata body
(the same area that calls get_cache_metadata_key using server_key, filters,
vc_detection_enabled, use_sysname, strip_domain) to also set use_sysname and
strip_domain in the metadata dict/object saved to storage, and do the same for
the second occurrence of this logic (the block around the later call that
mirrors lines 661-667); ensure the metadata keys exactly match the boolean flags
(use_sysname, strip_domain) so get_active_cached_searches() can list/restore
searches correctly.
Owner
Author
There was a problem hiding this comment.
Finding rejected — the use_sysname/strip_domain flags are already part of the cache key (via get_cache_metadata_key) for distinguishing
searches. The metadata body doesn't need them because get_active_cached_searches and the template don't use them — naming preferences are
loaded from user prefs, not from cached search metadata.
There was a problem hiding this comment.
@marcinpsk, understood — the cache key itself is sufficient to differentiate searches by naming mode, and since get_active_cached_searches and the template both source naming preferences from user prefs rather than the metadata payload, there's no need to store the flags there. Acknowledged.
✏️ Learnings added
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/import_utils/bulk_import.py:643-649
Timestamp: 2026-03-07T18:32:16.173Z
Learning: In `netbox_librenms_plugin/import_utils/bulk_import.py`, `process_device_filters` stores cache metadata (cached_at, cache_timeout, filters, vc_enabled, device_count) via `get_cache_metadata_key`. The `use_sysname` and `strip_domain` naming-mode flags intentionally do NOT need to be stored in the metadata body: they are already encoded into the cache key by `get_cache_metadata_key`, and `get_active_cached_searches` / the template never read them from the metadata — naming preferences are loaded from user prefs instead. Do not suggest adding use_sysname/strip_domain to the metadata dict.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_utils.py : `process_device_filters(filters, ...)` must fetch and validate devices from LibreNMS, returning a list
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/jobs.py : `FilterDevicesJob` background job: filters devices with VC detection. `job.data` keys must include `device_ids`, `total_processed`, `filters`, `server_key`, `vc_detection_enabled`, `cache_timeout`, `cached_at`, `completed`. Devices are cached individually via shared cache keys from `get_validated_device_cache_key()`
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`, `get_librenms_sync_device` Priority 1 loop (dict fast-path) only needs to guard against `None` and `bool` values in `raw_cf.get(server_key)`; string normalization and full validation are intentionally deferred to the Priority 2 loop which calls `get_librenms_device_id(member, server_key, auto_save=False)`. Do not suggest replacing the Priority 1 condition with a full `get_librenms_device_id` call — the two-pass design is intentional.
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 `netbox_librenms_plugin/views/sync/cables.py`, `SyncCablesView.post()` reads `server_key = request.POST.get("server_key") or self.librenms_api.server_key` and stores it as `self._post_server_key`. This is essential for server-scoped cache lookups in `get_cached_links_data` (which uses `self._post_server_key` to build the cache key). The `&server_key={server_key}` appended to the redirect URL is harmless (the sync page ignores it, resolving the active server from the global plugin setting), but it is an intentional, consistent pattern used across the codebase for future-proofing. Do not flag this as redundant or suggest removing the POST reading — doing so would break the cache namespace scoping for link data lookups.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T13:05:27.507Z
Learning: In `netbox_librenms_plugin/views/base/ip_addresses_view.py`, `_prefetch_netbox_data` already reads `server_key` from `self.librenms_api.server_key` and passes it to `get_librenms_device_id` when building the `interfaces_by_librenms_id` map. Do not flag `enrich_ip_data` or `_prefetch_netbox_data` as missing server context — per-server interface ID lookups are already handled correctly without needing additional server_key threading through the call chain.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:64-68
Timestamp: 2026-03-07T17:18:43.062Z
Learning: In netbox_librenms_plugin/views/sync/*.py (SyncInterfacesView, SyncCablesView, SyncIPAddressesView, SyncVLANsView), appending `&server_key={server_key}` to the POST-redirect URL is intentional and harmless. The sync GET handler resolves the active server from settings.selected_server via self.librenms_api.server_key and ignores the query param, so including it is a minor redundancy — not a bug. The critical part is reading server_key from request.POST and assigning it to self._post_server_key so cache lookups use the correct per-server namespace. Do not flag this redirect server_key append as propagating a non-consumed source of truth.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/jobs.py : `ImportDevicesJob` background job: imports devices and VMs. Must call `bulk_import_devices_shared()` for devices and `bulk_import_vms()` for VMs. `job.data` keys must include `imported_device_pks`, `imported_vm_pks`, `imported_libre_device_ids`, `imported_libre_vm_ids`, `server_key`, `total`, `success_count`, `failed_count`, `skipped_count`, `virtual_chassis_created`, `errors`, `completed`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/** : `BulkImportDevicesView` (POST) must execute import: background mode enqueues `ImportDevicesJob`; sync mode calls `bulk_import_devices()` + `bulk_import_vms()` and returns OOB row swaps with `HX-Trigger: closeModal`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-03T13:23:33.731Z
Learning: Applies to netbox_librenms_plugin/**/*.py : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T13:05:27.506Z
Learning: In `netbox_librenms_plugin/import_utils/device_operations.py`, the local variable `import_as_vm` is refreshed at line 487 via `import_as_vm = result["import_as_vm"]` (with an explanatory comment) before the branching logic at line 490. This ensures any VM-mode adjustments made during the VM detection blocks (lines ~293 and ~403, which set `result["import_as_vm"] = True`) are reflected in subsequent control flow. Do not flag the `import_as_vm` local variable as stale or suggest adding per-block assignments — the single refresh at line 487 is the intentional fix point.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/** : Result loading via `_load_job_results(job_id)` must read `job.data['device_ids']` and reconstruct devices from per-device cache using `get_validated_device_cache_key()`
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T16:59:45.385Z
Learning: In `netbox_librenms_plugin/views/imports/actions.py`, the concurrent write race condition in `DeviceConflictActionView` (where two requests for different devices could both pass `find_by_librenms_id()` before either writes) is a theoretical edge case in an admin-driven UI action, not a high-concurrency API endpoint. The existing `select_for_update()` on the target device row provides sufficient protection for the actual usage pattern. Do not suggest introducing a dedicated `DeviceLibreNMSIDMapping` model, DB advisory locks, or unique constraints for this scenario — the custom field JSON approach is a deliberate design choice and adding a DB model would require migrations and add complexity far beyond the value it provides.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/jobs.py,**/import_utils.py : Both synchronous and background modes must use `get_validated_device_cache_key()` from `import_utils.py` to generate cache keys, ensuring `_load_job_results()` in the list view can retrieve devices regardless of which mode produced them. Never hardcode cache key formats; always use the helper functions
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/views/imports/** : Import page supports synchronous mode (calls `process_device_filters()` directly, renders results inline) and background mode (enqueues `FilterDevicesJob`, returns `JsonResponse` with `job_id`/`job_pk`/`poll_url`, frontend polls and redirects on completion)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-03-03T13:24:00.657Z
Learning: Applies to **/import_utils.py : `bulk_import_devices_shared(devices, user, ...)` must be the shared implementation between sync and background import
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:604-612
Timestamp: 2026-03-07T11:09:04.002Z
Learning: In `netbox_librenms_plugin/utils.py`, `find_by_librenms_id(model, librenms_id, server_key)` intentionally merges the server-scoped JSON lookup (`custom_field_data__librenms_id__{server_key}`) and the legacy bare-int/str fallback (`custom_field_data__librenms_id`) into a single OR Q object and calls `.first()`. This is correct and intentional because: (1) librenms_id values are unique per LibreNMS device, so two different DB objects cannot hold the same ID in different formats simultaneously; (2) the migration workflow converts bare-int to `{server_key: id}` atomically, preventing a state where both forms coexist on separate objects; (3) splitting into two sequential queries would double DB hits on a hot path called for every port during cable enrichment; (4) string normalization is already covered by the `str(librenms_id)` variants on the legacy branches. Do not suggest splitting this into a two-step lookup.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:439-447
Timestamp: 2026-03-06T18:41:06.857Z
Learning: In netbox_librenms_plugin/views/sync/device_fields.py, `RemoveServerMappingView._normalize_librenms_mapping` is intentionally a local helper that converts any raw librenms_id CF value (int, numeric str, dict) into a full `{server_key: device_id}` dict for membership checks and key deletion. This is distinct from utils.py helpers (`get_librenms_device_id`, `set_librenms_device_id`, `migrate_legacy_librenms_id`, `find_by_librenms_id`) which all operate on a single server key; none return the full mapping dict. Do not flag `_normalize_librenms_mapping` as duplication of the utils layer.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:22-39
Timestamp: 2026-03-07T10:33:28.005Z
Learning: In `netbox_librenms_plugin/views/base/cables_view.py`, `_librenms_id_q(server_key, value)` intentionally combines `custom_field_data__librenms_id__{server_key}` (server-scoped JSON) and `custom_field_data__librenms_id` (legacy bare-int/str) in a single OR Q object. This is correct and safe because all callers scope queries to a specific device's interfaces (`obj.interfaces.filter(...)` or `chassis_member.interfaces.filter(...)`), so cross-object collisions cannot occur. The two formats represent the same logical identity stored differently (legacy vs. migrated). Do not suggest splitting this into a two-step lookup — it would add unnecessary DB round-trips for no correctness benefit. The same OR pattern is used in `find_by_librenms_id` in `utils.py`.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:241-244
Timestamp: 2026-03-07T10:40:38.106Z
Learning: In netbox_librenms_plugin, `set_librenms_device_id` is used for both Device/VM objects (to store the LibreNMS device ID) and Interface/VMInterface objects (to store the LibreNMS port ID). The legacy bare-int guard in `set_librenms_device_id` is only relevant for Device/VM objects where a pre-existing bare integer might exist from before multi-server support. For Interface/VMInterface objects, the `librenms_id` custom field starts empty (null/{}) because `port_id` is always freshly written from the LibreNMS API JSON response; there is no legacy bare-int migration concern for interfaces. Do not flag the warning-log path on interfaces as a silent no-op bug.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:573-588
Timestamp: 2026-03-07T13:14:08.292Z
Learning: In `netbox_librenms_plugin/views/sync/device_fields.py`, `ConvertLegacyLibreNMSIdView.post()` intentionally uses `self.librenms_api.server_key` (the active server from the global plugin setting) rather than reading `server_key` from `request.POST`. The "Convert Legacy ID" form template (`librenms_sync_base.html`, lines ~160-175) only submits `object_type` and CSRF token — no `server_key` field — because conversion always targets the currently active server the user is viewing. Serial verification via `get_device_info()` also correctly queries the active server. Do not flag this as a missing or wrong server_key; the active server context is the correct and only sensible target for the legacy-ID conversion.
Comment on lines
+69
to
+70
| [dependency-groups] | ||
| dev = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files that may define or consume development dependencies =="
fd -HI '^(pyproject\.toml|README(\..*)?$|requirements.*\.txt|tox\.ini|\.pre-commit-config\.yaml|uv\.lock|Pipfile|Pipfile\.lock)$' \
-x sh -c 'printf "\n--- %s ---\n" "$1"; rg -n -S "(--group dev|dependency-groups|pytest|pytest-django|ruff|pre-commit|tox|coverage|requirements-dev)" "$1" || true' sh {}
echo
echo "== Direct matches for group-based install commands or docs =="
rg -n --hidden -S '(--group[ =]dev|uv sync .*--group dev|pip install .*--group dev|dependency-groups)' .Repository: marcinpsk/netbox-librenms-plugin
Length of output: 1004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== GitHub Actions workflows =="
fd -HI '\.github/workflows' -type f -x cat -n {} || echo "No GitHub Actions workflows found"
echo
echo "== GitLab CI =="
test -f .gitlab-ci.yml && cat -n .gitlab-ci.yml || echo "No GitLab CI found"
echo
echo "== Installation instructions in README files =="
find . -name "README*" -type f | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && grep -n -i "install\|setup\|develop\|group dev\|requirements" {} || true' | head -100
echo
echo "== Check pyproject.toml for how dep groups are defined (full section) =="
sed -n '69,100p' pyproject.toml
echo
echo "== Check for any uv.toml or uvconfig =="
test -f uv.toml && cat -n uv.toml || echo "No uv.toml found"Repository: marcinpsk/netbox-librenms-plugin
Length of output: 6201
Populate or remove the empty dev group.
dev = [] is a no-op. The repository currently sources dev dependencies from requirements_dev.txt (which contains pytest and pytest-django), and no tooling or documentation references the new [dependency-groups] section. Either migrate dev dependencies into this group and update build/CI scripts to use --group dev, or remove this section until adoption is planned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 69 - 70, The pyproject.toml contains an empty
dependency group declaration ([dependency-groups] with dev = []), which is a
no-op; either remove this section or populate the dev group and update CI/build
invocation. To fix: either delete the [dependency-groups] entry entirely if you
will continue using requirements_dev.txt, or migrate the dev packages (e.g.,
pytest, pytest-django) into the dev group in pyproject.toml and update CI/build
scripts to install with the --group dev flag (and remove or stop sourcing
requirements_dev.txt). Ensure you update references in CI/config to use --group
dev if you choose migration.
marcinpsk
force-pushed
the
pr/librems-id-multi-fixes
branch
from
March 7, 2026 22:15
204be4b to
7e34e22
Compare
marcinpsk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
744-752:⚠️ Potential issue | 🟠 MajorPass the constructed API client into self-validation.
When
validationis omitted, this path threads onlyserver_key; the new chassis-inventory fallback and VC detection never run becauseapiisNoneinsidevalidate_device_for_import(). That makesimport_single_device(validation=None)behave worse than callers that precompute validation.Suggested fix
validation = validate_device_for_import( libre_device, + api=api, use_sysname=use_sysname_opt, strip_domain=strip_domain_opt, server_key=api.server_key, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 744 - 752, The import path in import_single_device where validation is None only passes server_key into validate_device_for_import, so the constructed API client (api) isn't available and chassis-inventory fallback / VC detection won't run; update the call in device_operations.py to pass the constructed API client (api) into validate_device_for_import (e.g., add api=api or api_client=api depending on the function signature) alongside server_key, so validate_device_for_import and its internal logic (chassis-inventory fallback and VC detection) receive the API client when validation is computed lazily.
♻️ Duplicate comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
251-286:⚠️ Potential issue | 🟠 MajorReject boolean
device_idbefore any lookup runs.This is the same coercion hole fixed in the VM create path:
int(True)becomes1, so a malformed payload can falsely match an existing Device/VM and drive follow-up lookups against the wrong LibreNMS object. Bail out once, near the initialdevice_idextraction, before reusing it anywhere else.Also applies to: 317-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 251 - 286, The code extracts librenms_id = libre_device.get("device_id") and then coerces it into int later, which lets boolean True/False become 1/0 and produce false matches; add an early validation right after obtaining librenms_id to explicitly reject boolean values (and other non-integer-safe types) and return/raise an error before any lookups run (affecting the VM path using find_by_librenms_id and the Device path later around the same check), e.g., check isinstance(librenms_id, bool) or ensure it is a string of digits or an actual int, and bail out if invalid so neither find_by_librenms_id(VirtualMachine, int(librenms_id), server_key) nor the device lookup at the later block (lines ~317-320) will be reached with a coerced boolean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 767-770: The IP verify payload is missing object_type which causes
SingleIPAddressVerifyView to mis-identify objects; update the payload
construction in librenms_sync.js (the block that sets device_id, ip_address,
vrf_id, server_key) to include object_type, sourcing it from the page context
(e.g. an existing variable like objectType or a DOM element such as
document.getElementById('object_type')?.value ||
document.getElementById('object-type')?.value) and defaulting to null if not
present so the server receives the correct object type for verification.
- Around line 637-643: When showing the VLAN-override modal, clear any stale
error state before reuse: locate the code creating/reusing alertEl (querying
'.vlan-override-error' on modalEl) and either remove any existing alert element
or reset its textContent/hidden state before making the next save request;
ensure the logic in the block that appends alertEl (using
modalEl.querySelector('.modal-body') and alertEl.className) first clears
existing alerts or sets alertEl.textContent = ''/hidden so the old error does
not appear when the modal is reopened.
In `@netbox_librenms_plugin/tests/test_librenms_id.py`:
- Line 184: Remove the redundant local import of MagicMock in the test
(currently re-imported at the top of the file) — delete the line "from
unittest.mock import MagicMock" in the test block so the module-level MagicMock
import is used; verify no other local references rely on a different import
name.
- Around line 223-224: The import "from unittest.mock import MagicMock" in
tests/test_librenms_id.py is redundant because MagicMock is already imported at
module scope; remove that duplicate import line and rely on the module-level
MagicMock for all tests (verify no local redefinitions or shadowing of MagicMock
in functions like the tests referencing Q or other mocks).
In `@netbox_librenms_plugin/tests/test_vm_operations.py`:
- Around line 104-128: The test contains a redundant local import "from
unittest.mock import patch" inside test_server_key_stored_in_custom_field;
remove that local import and rely on the module-level patch import, leaving the
rest of the test (including usage of patch in the context managers, the mocked
VirtualMachine, transaction.atomic, and set_librenms_device_id around
create_vm_from_librenms) unchanged so the assertions on mock_setter and
mock_vm.save still run.
In `@netbox_librenms_plugin/utils.py`:
- Around line 649-651: The branch that currently only accepts legacy string IDs
when cf_value.isdigit() is too restrictive; instead mirror the reader by
attempting to parse strings into ints (handling cases like " 42 " or "+42"). In
the block around get_librenms_device_id() in utils.py, replace the isdigit()
guard with an isinstance(cf_value, str) branch that tries int(cf_value) inside a
try/except ValueError and assigns int_value on success, falling through to the
existing else behavior on failure; this ensures the migration path accepts the
same legacy string forms the reader already does.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 292-308: The current use of Platform.objects.create(...) bypasses
Django model validation so the ValidationError except block is never hit; change
the block in the transaction.atomic() to instantiate
Platform(name=platform_name, manufacturer=manufacturer), call
instance.full_clean() to run model validators, then call instance.save(),
keeping the existing ValidationError except handler (and
transaction.set_rollback(True), logger.error, messages.error, and redirect) so
validation failures are caught and handled as intended; ensure you retain the
surrounding transaction.atomic() and any IntegrityError fallback elsewhere.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 744-752: The import path in import_single_device where validation
is None only passes server_key into validate_device_for_import, so the
constructed API client (api) isn't available and chassis-inventory fallback / VC
detection won't run; update the call in device_operations.py to pass the
constructed API client (api) into validate_device_for_import (e.g., add api=api
or api_client=api depending on the function signature) alongside server_key, so
validate_device_for_import and its internal logic (chassis-inventory fallback
and VC detection) receive the API client when validation is computed lazily.
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 251-286: The code extracts librenms_id =
libre_device.get("device_id") and then coerces it into int later, which lets
boolean True/False become 1/0 and produce false matches; add an early validation
right after obtaining librenms_id to explicitly reject boolean values (and other
non-integer-safe types) and return/raise an error before any lookups run
(affecting the VM path using find_by_librenms_id and the Device path later
around the same check), e.g., check isinstance(librenms_id, bool) or ensure it
is a string of digits or an actual int, and bail out if invalid so neither
find_by_librenms_id(VirtualMachine, int(librenms_id), server_key) nor the device
lookup at the later block (lines ~317-320) will be reached with a coerced
boolean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a371949e-c132-43cd-b595-6a75f237d59a
📒 Files selected for processing (15)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/device_fields.py
| coexist — matching only one would silently miss devices stored in the other | ||
| format. | ||
| """ | ||
| from unittest.mock import MagicMock |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant import of MagicMock.
MagicMock is already imported at module level (line 7). Consider removing this local import.
🧹 Proposed fix
def test_queries_server_key_and_legacy_integer(self):
"""find_by_librenms_id() issues a Q that covers both the JSON server-key branch
and the legacy bare-int branch in a single filter() call.
We inspect the Q object's children directly because the two branches must
coexist — matching only one would silently miss devices stored in the other
format.
"""
- from unittest.mock import MagicMock
from django.db.models import Q
from netbox_librenms_plugin.utils import find_by_librenms_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tests/test_librenms_id.py` at line 184, Remove the
redundant local import of MagicMock in the test (currently re-imported at the
top of the file) — delete the line "from unittest.mock import MagicMock" in the
test block so the module-level MagicMock import is used; verify no other local
references rely on a different import name.
Owner
Author
There was a problem hiding this comment.
Reject. Removing redundant imports is a cosmetic cleanup, not a bug fix. The existing code works correctly. We're not doing style-only
changes.
There was a problem hiding this comment.
Understood, @marcinpsk. I'll skip style-only suggestions like redundant import cleanup going forward and focus on correctness issues.
✏️ Learnings added
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: In marcinpsk/netbox-librenms-plugin, do not flag or suggest style-only changes (e.g., removing redundant imports, cosmetic cleanup) in code reviews. Only raise issues that affect correctness, functionality, or security.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:604-612
Timestamp: 2026-03-07T11:09:04.002Z
Learning: In `netbox_librenms_plugin/utils.py`, `find_by_librenms_id(model, librenms_id, server_key)` intentionally merges the server-scoped JSON lookup (`custom_field_data__librenms_id__{server_key}`) and the legacy bare-int/str fallback (`custom_field_data__librenms_id`) into a single OR Q object and calls `.first()`. This is correct and intentional because: (1) librenms_id values are unique per LibreNMS device, so two different DB objects cannot hold the same ID in different formats simultaneously; (2) the migration workflow converts bare-int to `{server_key: id}` atomically, preventing a state where both forms coexist on separate objects; (3) splitting into two sequential queries would double DB hits on a hot path called for every port during cable enrichment; (4) string normalization is already covered by the `str(librenms_id)` variants on the legacy branches. Do not suggest splitting this into a two-step lookup.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:22-39
Timestamp: 2026-03-07T10:33:28.005Z
Learning: In `netbox_librenms_plugin/views/base/cables_view.py`, `_librenms_id_q(server_key, value)` intentionally combines `custom_field_data__librenms_id__{server_key}` (server-scoped JSON) and `custom_field_data__librenms_id` (legacy bare-int/str) in a single OR Q object. This is correct and safe because all callers scope queries to a specific device's interfaces (`obj.interfaces.filter(...)` or `chassis_member.interfaces.filter(...)`), so cross-object collisions cannot occur. The two formats represent the same logical identity stored differently (legacy vs. migrated). Do not suggest splitting this into a two-step lookup — it would add unnecessary DB round-trips for no correctness benefit. The same OR pattern is used in `find_by_librenms_id` in `utils.py`.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/tests/test_import_utils.py:1345-1366
Timestamp: 2026-03-07T18:19:35.011Z
Learning: In `netbox_librenms_plugin/tests/test_import_utils.py`, the `_setup_librenms_id_match` helper's `device_filter` detects LibreNMS ID Q-object calls using `"librenms_id" in str(arg)`. Python's `str()` on a Django Q object traverses the full node tree and includes all field names from nested Q children, so this check reliably identifies librenms_id queries regardless of Q nesting depth. Do not flag this as a false negative due to nested Q nodes.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/imports/actions.py:1283-1295
Timestamp: 2026-03-07T12:29:31.078Z
Learning: In `netbox_librenms_plugin/views/imports/actions.py`, the `migrate_librenms_id` action's conflict check uses `find_by_librenms_id(existing_model, cf_locked_int, server_key)` (not a hand-rolled Q) to detect whether another object of the same model already owns the ID before migrating. Cross-model conflict checks (Device vs VirtualMachine) are intentionally not performed here because migration only converts storage format (bare int → {server_key: id}) on the same object; it does not change associations across models. Do not suggest adding cross-model conflict checks to the migrate_librenms_id action.
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`, `get_librenms_sync_device` Priority 1 loop (dict fast-path) only needs to guard against `None` and `bool` values in `raw_cf.get(server_key)`; string normalization and full validation are intentionally deferred to the Priority 2 loop which calls `get_librenms_device_id(member, server_key, auto_save=False)`. Do not suggest replacing the Priority 1 condition with a full `get_librenms_device_id` call — the two-pass design is intentional.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T16:59:45.385Z
Learning: In `netbox_librenms_plugin/views/sync/device_fields.py`, the `ConvertLegacyLibreNMSIdView` collision check pattern `match = find_by_librenms_id(model, librenms_id, server_key); conflict = match is not None and match.pk != locked.pk` is correct and complete. `find_by_librenms_id` returns the first object holding that ID; if it returns the locked row itself (`match.pk == locked.pk`), `conflict` is `False` — no conflict. If it returns a different object, `conflict` is `True`. Do not suggest adding `exclude_pk` to `find_by_librenms_id` or switching to `.exclude().exists()` — the current two-line pattern is correct and readable.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T16:59:45.385Z
Learning: In `netbox_librenms_plugin/tests/test_ip_verify.py`, the `_mock_device(pk=...)` local helper function is intentionally purpose-built for cache-key testing and must not be replaced with the `mock_netbox_device` conftest fixture. The conftest fixture hardcodes `name="test-device"` and has no `pk`, while tests in `test_ip_verify.py` require distinct `pk` values (e.g., 42, 5, 7) and `get_absolute_url` returns keyed to those PKs. Replacing it would require post-fixture mutation and add unnecessary attributes (`primary_ip4`, `cf`, etc.). Do not flag the local `_mock_device` helper as redundant with `mock_netbox_device`.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:573-588
Timestamp: 2026-03-07T13:14:08.292Z
Learning: In `netbox_librenms_plugin/views/sync/device_fields.py`, `ConvertLegacyLibreNMSIdView.post()` intentionally uses `self.librenms_api.server_key` (the active server from the global plugin setting) rather than reading `server_key` from `request.POST`. The "Convert Legacy ID" form template (`librenms_sync_base.html`, lines ~160-175) only submits `object_type` and CSRF token — no `server_key` field — because conversion always targets the currently active server the user is viewing. Serial verification via `get_device_info()` also correctly queries the active server. Do not flag this as a missing or wrong server_key; the active server context is the correct and only sensible target for the legacy-ID conversion.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T22:43:01.893Z
Learning: In `netbox_librenms_plugin/import_utils/device_operations.py`, do not suggest adding an upstream boolean guard on `librenms_id = libre_device.get("device_id")` before calls to `find_by_librenms_id`. `find_by_librenms_id` (in `netbox_librenms_plugin/utils.py` around line 612) already has an internal boolean guard that returns None for boolean inputs. Even if `int(True)` coercion succeeds and returns 1, the subsequent `find_by_librenms_id` call rejects it internally. Adding a redundant pre-call boolean check would be dead code.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/tests/test_sync_devices.py:6-19
Timestamp: 2026-03-07T17:06:37.167Z
Learning: In `netbox_librenms_plugin/tests/test_sync_devices.py`, the module-level `_make_view(cls_name, module_path)` and `_make_field_view(cls_name)` helpers are intentionally purpose-built minimal mocks that create view instances via `object.__new__` and wire only `_librenms_api`, `server_key`, and `request`. They are NOT redundant with conftest fixtures (`mock_librenms_api`, `mock_netbox_device`, etc.) because: (1) they don't need the extra attributes those fixtures carry; (2) replacing them would require awkward post-fixture overrides; (3) coupling to conftest would cause these tests to break on unrelated conftest changes. Do not flag `_make_view` or `_make_field_view` as redundant with conftest fixtures or suggest replacing them.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Reuse fixtures from `tests/conftest.py` instead of creating ad-hoc mocks: Configuration (`mock_multi_server_config`, `mock_legacy_config`), API client (`mock_librenms_api`), NetBox objects (`mock_netbox_device`, `mock_netbox_vm`, `mock_netbox_site`, `mock_netbox_platform`, `mock_netbox_device_type`, `mock_netbox_device_role`, `mock_netbox_cluster`, `mock_netbox_rack`), HTTP responses (`mock_response_factory`, `mock_success_response`, `mock_device_response`, `mock_error_response`, `mock_auth_error_response`), and Import workflow (`sample_librenms_device`, `sample_librenms_device_minimal`, `sample_validation_state`, `sample_validation_state_vm`).
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/tests/test_cable_verify.py:109-130
Timestamp: 2026-03-07T13:07:26.124Z
Learning: In `netbox_librenms_plugin/tests/test_cable_verify.py`, `test_raw_keys_match_prepare_context` intentionally uses `inspect.getsource()` plus string matching to verify that `BaseCableTableView._prepare_context` and `SingleCableVerifyView.post` both define the same `_raw_keys` set. This is a deliberate structural/drift-detection test. Do not flag it as fragile or suggest extracting a shared constant or using runtime comparison — the local variables are not accessible at runtime without extensive mocking, and a shared constant would add unnecessary coupling.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T13:05:27.507Z
Learning: In `netbox_librenms_plugin/views/base/ip_addresses_view.py`, `_prefetch_netbox_data` already reads `server_key` from `self.librenms_api.server_key` and passes it to `get_librenms_device_id` when building the `interfaces_by_librenms_id` map. Do not flag `enrich_ip_data` or `_prefetch_netbox_data` as missing server context — per-server interface ID lookups are already handled correctly without needing additional server_key threading through the call chain.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/test_netbox_librenms_plugin.py : `test_netbox_librenms_plugin.py` is an empty placeholder — do not add tests there.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:241-244
Timestamp: 2026-03-07T10:40:38.106Z
Learning: In netbox_librenms_plugin, `set_librenms_device_id` is used for both Device/VM objects (to store the LibreNMS device ID) and Interface/VMInterface objects (to store the LibreNMS port ID). The legacy bare-int guard in `set_librenms_device_id` is only relevant for Device/VM objects where a pre-existing bare integer might exist from before multi-server support. For Interface/VMInterface objects, the `librenms_id` custom field starts empty (null/{}) because `port_id` is always freshly written from the LibreNMS API JSON response; there is no legacy bare-int migration concern for interfaces. Do not flag the warning-log path on interfaces as a silent no-op bug.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:439-447
Timestamp: 2026-03-06T18:41:06.857Z
Learning: In netbox_librenms_plugin/views/sync/device_fields.py, `RemoveServerMappingView._normalize_librenms_mapping` is intentionally a local helper that converts any raw librenms_id CF value (int, numeric str, dict) into a full `{server_key: device_id}` dict for membership checks and key deletion. This is distinct from utils.py helpers (`get_librenms_device_id`, `set_librenms_device_id`, `migrate_legacy_librenms_id`, `find_by_librenms_id`) which all operate on a single server key; none return the full mapping dict. Do not flag `_normalize_librenms_mapping` as duplication of the utils layer.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-03T13:23:33.731Z
Learning: Applies to netbox_librenms_plugin/**/*.py : Use `LibreNMSAPI.get_librenms_id()` instead of directly accessing the `librenms_id` custom field when mapping Devices/VMs to LibreNMS
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-03-03T13:24:51.364Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-03T13:23:33.731Z
Learning: Applies to netbox_librenms_plugin/**/*.py : Centralize validation state mutation during import using `import_validation_helpers.py` for role/cluster/rack assignment, issue removal, and status recalculation
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:64-68
Timestamp: 2026-03-07T17:18:43.062Z
Learning: In netbox_librenms_plugin/views/sync/*.py (SyncInterfacesView, SyncCablesView, SyncIPAddressesView, SyncVLANsView), appending `&server_key={server_key}` to the POST-redirect URL is intentional and harmless. The sync GET handler resolves the active server from settings.selected_server via self.librenms_api.server_key and ignores the query param, so including it is a minor redundancy — not a bug. The critical part is reading server_key from request.POST and assigning it to self._post_server_key so cache lookups use the correct per-server namespace. Do not flag this redirect server_key append as propagating a non-consumed source of truth.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T16:59:45.385Z
Learning: In `netbox_librenms_plugin/views/imports/actions.py`, the concurrent write race condition in `DeviceConflictActionView` (where two requests for different devices could both pass `find_by_librenms_id()` before either writes) is a theoretical edge case in an admin-driven UI action, not a high-concurrency API endpoint. The existing `select_for_update()` on the target device row provides sufficient protection for the actual usage pattern. Do not suggest introducing a dedicated `DeviceLibreNMSIDMapping` model, DB advisory locks, or unique constraints for this scenario — the custom field JSON approach is a deliberate design choice and adding a DB model would require migrations and add complexity far beyond the value it provides.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T12:23:02.478Z
Learning: In `netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`, the `cluster_<device_id>` hidden input inside the legacy ID migration form block (around lines 415-417) is intentionally dead code. The `migrate_librenms_id` action in `views/imports/actions.py` only converts the `librenms_id` custom field from a bare integer to a per-server dict format; it never reads nor processes cluster assignment. Do not flag this hidden input as a missing cluster-preservation bug — cluster assignment is not part of the legacy ID migration flow.
Owner
Author
Reject. find_by_librenms_id already has a boolean guard at line 612 that returns None for boolean inputs. Additionally, on this branch the call at line 286 wraps it in int(librenms_id) with a try/except (ValueError, TypeError) — but even if int(True) succeeds (returning 1), find_by_librenms_id would still reject it at its own boolean guard. On the pr/librenms-id-multi-server branch (other PR), the call passes librenms_id directly (no int() coercion), and find_by_librenms_id handles the boolean check internally. |
|
Tip For best results, initiate chat on the files or code changes.
( ˘･_･˘ ) ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/views/base/cables_view.py (1)
361-365:⚠️ Potential issue | 🟡 MinorPreserve
server_keyin the no-data paths.The normal path returns
server_key, but both empty-state fallbacks drop it. That leaves the template/JS without the server context this page was rendered for and makes the next action fall back to the current global server instead.Suggested fix
def get_context_data(self, request, obj): """Get the context data for the cable sync view.""" context = self._prepare_context(request, obj, fetch_fresh=False) if context is None: # No data found; return context with empty table - context = {"table": None, "object": obj, "cache_expiry": None} + context = { + "table": None, + "object": obj, + "cache_expiry": None, + "server_key": self.librenms_api.server_key, + } return context ... if context is None: messages.error(request, "No links found in LibreNMS") return render( request, self.partial_template_name, - {"cable_sync": {"object": obj, "table": None, "cache_expiry": None}}, + { + "cable_sync": { + "object": obj, + "table": None, + "cache_expiry": None, + "server_key": self.librenms_api.server_key, + } + }, )Also applies to: 372-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/cables_view.py` around lines 361 - 365, The empty-state branches drop server_key causing templates/JS to lose server context; update the fallback contexts in the method that calls self._prepare_context (the block creating {"table": None, "object": obj, "cache_expiry": None}) to include the current server_key (preserve whatever server_key variable/value is available in scope) so both the early-return and the other empty-state fallback (around the similar block at lines 372-378) return {"table": None, "object": obj, "cache_expiry": None, "server_key": server_key}; ensure you reference the same server_key identifier used elsewhere in this view so templates and client code receive the correct server context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/tests/test_librenms_id.py`:
- Around line 283-303: Add tests asserting migrate_legacy_librenms_id rejects
non-canonical numeric strings: create MagicMock obj.custom_field_data with
"librenms_id" set to "+42" and another with " 42 " and call
migrate_legacy_librenms_id(obj, "<env>"); assert the function returns False,
obj.custom_field_data remains unchanged (still the original string), and
obj.save is not called. Reference the existing test functions
test_migrates_string_digit_legacy_id and test_returns_false_for_non_digit_string
to mirror their structure but use the inputs "+42" and " 42 " to ensure
.isdigit() behavior is enforced.
In `@netbox_librenms_plugin/tests/test_reviewer_fixes.py`:
- Around line 261-329: Tests only check that server_key is passed to
get_librenms_sync_device but not that the same server_key is used to build the
cache key; update the tests for SingleCableVerifyView.post to also assert
get_cache_key (or cache.get) is called with the expected server_key so
regressions where get_librenms_sync_device gets the posted key but cache lookup
uses self._librenms_api.server_key will fail. Concretely, in the two tests add
an assertion referencing get_cache_key or inspect mock_cache.get.call_args to
verify the cache key/path contains "production" (first test) and
"fallback-server" (second test); use the existing mocks for
patch("...cables_view.cache") and the view instance created via
object.__new__(SingleCableVerifyView) to check mock_cache.get was called with a
key built from the same server_key passed into get_librenms_sync_device.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 83-94: The current links cache stores request-specific local_port
labels computed via get_interface_name_field(request) and local_ports_map (from
ports_data.get("ports")), causing stale interface names across users; change the
caching strategy in the logic that builds links so you only cache local_port_id
(or otherwise exclude per-request labels) and rebuild the human-readable port
name per request using get_interface_name_field(request) and local_ports_map, or
alternatively include interface_name_field in the cache key so cached entries
are partitioned by naming preference; ensure any calls that rely on the label
(e.g., get_virtual_chassis_member) use the per-request rebuilt label rather than
a shared cached string.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 300-309: The except ValidationError block that handles platform
creation (the one that calls transaction.set_rollback(True), logger.error(...,
exc_info=True) and messages.error(...)) should surface the actual validation
failure instead of always saying “slug collision”; update the messages.error
call to include the ValidationError details (e.g., use str(e) or e.messages)
and/or append the specific field errors so users get the real remediation hint
while keeping the existing logger.error and exc_info=True for full traceback;
reference the platform_name and pk in the user-facing message along with the
validation text to make the error actionable.
- Around line 638-644: The except block currently returns the raw exception
string to the UI; change it so ValidationError still shows the specific message
via messages.error(request, str(exc)) but for other Exception types do not
expose exc to users: call transaction.set_rollback(True), log the full exception
server-side (e.g., logger.exception("Failed saving converted librenms_id for
%s/%s", object_type, pk)) and send a generic messages.error(request, "Failed to
save converted librenms_id; contact administrator."), then return
self._sync_url(object_type, pk); keep references to locked.full_clean(),
locked.save(), ValidationError, transaction.set_rollback(True), messages.error
and self._sync_url to locate the code.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 361-365: The empty-state branches drop server_key causing
templates/JS to lose server context; update the fallback contexts in the method
that calls self._prepare_context (the block creating {"table": None, "object":
obj, "cache_expiry": None}) to include the current server_key (preserve whatever
server_key variable/value is available in scope) so both the early-return and
the other empty-state fallback (around the similar block at lines 372-378)
return {"table": None, "object": obj, "cache_expiry": None, "server_key":
server_key}; ensure you reference the same server_key identifier used elsewhere
in this view so templates and client code receive the correct server context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a63cc6c-1758-4229-aa87-c64556a83a00
📒 Files selected for processing (6)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/sync/device_fields.py
Comment on lines
+638
to
+644
| try: | ||
| locked.full_clean() | ||
| locked.save() | ||
| except (ValidationError, Exception) as exc: | ||
| transaction.set_rollback(True) | ||
| messages.error(request, f"Failed to save converted librenms_id: {exc}") | ||
| return self._sync_url(object_type, pk) |
There was a problem hiding this comment.
Don’t echo unexpected save exceptions back to the UI.
This branch currently exposes str(exc) for any non-validation failure from full_clean()/save(). That can leak internal backend details. Keep the ValidationError path if needed, but log unexpected exceptions and return a generic message.
🔒 Suggested fix
try:
locked.full_clean()
locked.save()
- except (ValidationError, Exception) as exc:
+ except ValidationError as exc:
transaction.set_rollback(True)
messages.error(request, f"Failed to save converted librenms_id: {exc}")
return self._sync_url(object_type, pk)
+ except Exception:
+ transaction.set_rollback(True)
+ logger.exception(
+ "Unexpected error saving converted LibreNMS ID for %s pk=%s",
+ model.__name__,
+ pk,
+ )
+ messages.error(request, "An unexpected error occurred while saving the converted librenms_id.")
+ return self._sync_url(object_type, pk)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/device_fields.py` around lines 638 - 644,
The except block currently returns the raw exception string to the UI; change it
so ValidationError still shows the specific message via messages.error(request,
str(exc)) but for other Exception types do not expose exc to users: call
transaction.set_rollback(True), log the full exception server-side (e.g.,
logger.exception("Failed saving converted librenms_id for %s/%s", object_type,
pk)) and send a generic messages.error(request, "Failed to save converted
librenms_id; contact administrator."), then return self._sync_url(object_type,
pk); keep references to locked.full_clean(), locked.save(), ValidationError,
transaction.set_rollback(True), messages.error and self._sync_url to locate the
code.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/views/base/cables_view.py (1)
405-421:⚠️ Potential issue | 🟠 MajorValidate
device_idbefore the ORM lookup.
json.loads()will deserializetrue/falseto booleans, and Line 421 will treatpk=Trueaspk=1. That reintroduces the bool→int coercion bug this PR is hardening elsewhere. Reject booleans and non-integer-safe values before callingget_object_or_404().Proposed fix
def post(self, request): data = json.loads(request.body) selected_device_id = data.get("device_id") local_port_id = data.get("local_port_id") # Read server_key from POST so we use the exact server the user was viewing server_key = data.get("server_key") or self.librenms_api.server_key + + if isinstance(selected_device_id, bool): + return JsonResponse({"status": "error", "message": "Invalid device_id"}, status=400) + if isinstance(selected_device_id, str): + if not selected_device_id.isdigit(): + return JsonResponse({"status": "error", "message": "Invalid device_id"}, status=400) + selected_device_id = int(selected_device_id) + elif not isinstance(selected_device_id, int): + return JsonResponse({"status": "error", "message": "Invalid device_id"}, status=400) formatted_row = { "local_port": "", "remote_port": "", "remote_device": "",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/base/cables_view.py` around lines 405 - 421, The post method currently passes selected_device_id directly to get_object_or_404 (Device), allowing JSON booleans or other non-integer-safe values to be treated as ints; before calling get_object_or_404(Device, pk=...), validate selected_device_id: ensure it's an integer-safe value (e.g., an int type or a digit-only string that can be safely cast and positive), reject booleans and non-numeric strings by returning an appropriate error response (400) or skipping lookup, then convert to an int and call get_object_or_404 with that validated id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/tests/test_librenms_id.py`:
- Around line 439-493: Add a regression test to ensure legacy bare numeric
strings are treated like bare ints and block writes: create a test (similar to
test_legacy_bare_int_blocks_write) that sets obj.custom_field_data =
{"librenms_id": "7"} (or "42"), calls set_librenms_device_id(obj, 99,
server_key="secondary"), and asserts the original string value is preserved
(i.e., obj.custom_field_data["librenms_id"] == "7") to prevent silent migration;
reference set_librenms_device_id in the test.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 405-421: The post method currently passes selected_device_id
directly to get_object_or_404 (Device), allowing JSON booleans or other
non-integer-safe values to be treated as ints; before calling
get_object_or_404(Device, pk=...), validate selected_device_id: ensure it's an
integer-safe value (e.g., an int type or a digit-only string that can be safely
cast and positive), reject booleans and non-numeric strings by returning an
appropriate error response (400) or skipping lookup, then convert to an int and
call get_object_or_404 with that validated id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62c5d3be-b8aa-409a-a93d-4136a0c02403
📒 Files selected for processing (6)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.py
Owner
Author
This is already fixed. Both fallback contexts include server_key |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 258-269: The naming metadata currently sets "source" to "sysname"
or "hostname" even when both _raw_sysname and _raw_hostname are empty; update
the logic around _source (used when building result["naming_criteria"]) to
explicitly set _source = "device_id" (or "device-{device_id}") when both
_raw_sysname and _raw_hostname are falsy so the fallback to device_id performed
by _determine_device_name() is correctly recorded; reference the existing
variables use_sysname, _raw_sysname, _raw_hostname, _source and device_id when
making this change.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 898-918: The function that builds per-server info
(_build_id_server_info / the loop reading cf_value =
existing_device.custom_field_data.get("librenms_id")) is including raw mapping
values like True, None, or non-digit strings; change the loop to validate and
skip invalid IDs before appending to result: for each (sk, did) ensure did is a
valid numeric device id (e.g., isinstance(did, int) or (isinstance(did, str) and
did.isdigit())); if not, continue the loop; keep the existing display_name
lookup logic and return result or None as before.
- Around line 1031-1035: Reject boolean payloads for librenms_id before
attempting integer coercion: in the block that reads
libre_device.get("device_id") (variable librenms_id) add an explicit check for
isinstance(librenms_id, bool) and return HttpResponse("Invalid or missing
LibreNMS device_id in payload", status=400) if true, then proceed with the
existing try/except int(librenms_id) conversion; this prevents True/False
becoming 1/0 and affecting set_librenms_device_id(), migrate_librenms_id(), and
find_by_librenms_id() logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28f40597-f77c-4446-aa3d-1ddc6e3f3168
📒 Files selected for processing (4)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/imports/actions.py
marcinpsk
force-pushed
the
pr/librems-id-multi-fixes
branch
2 times, most recently
from
March 8, 2026 11:48
678b07e to
eb56787
Compare
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
from
March 9, 2026 13:02
f260db7 to
b271013
Compare
marcinpsk
force-pushed
the
pr/librems-id-multi-fixes
branch
from
March 9, 2026 13:07
fc389d2 to
9d56fe9
Compare
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
from
March 9, 2026 18:47
b271013 to
55d9d70
Compare
marcinpsk
force-pushed
the
pr/librems-id-multi-fixes
branch
from
March 9, 2026 18:53
8d1e698 to
c1796e9
Compare
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
2 times, most recently
from
March 9, 2026 19:58
50fd794 to
2e52600
Compare
… sync delegation list.py: Check validated-cache metadata (which includes use_sysname and strip_domain in its key) instead of raw device cache when deciding whether to short-circuit the background job. A naming preference change now correctly shows the cache as cold and routes through the background path instead of doing synchronous re-validation. librenms_sync_view.py: Always delegate VC device resolution to get_librenms_sync_device() instead of first checking get_librenms_device_id(). In a partially migrated VC, a member with an explicit per-server dict is preferred over one with a legacy bare-int — the previous guard could pick the wrong member. Includes 5 new tests covering both fixes.
…gement
Replace bare-integer librenms_id custom field with a per-server JSON dict
format (e.g., {"primary": 42, "secondary": 99}) to support multi-server
LibreNMS deployments.
Key changes:
- Add get/set/find/migrate helpers for server-scoped librenms_id in utils.py
- Server-aware cache keys across all sync views (interfaces, cables, VLANs, IPs)
- Legacy bare-int IDs act as universal fallback for any server_key
- Convert ID badge on sync page for migrating legacy IDs with serial verification
- RemoveServerMappingView and ConvertLegacyLibreNMSIdView with collision checks
- Bool rejection guards on all librenms_id helpers (bool is subclass of int)
- CSRF token and response.ok guard in cable verify JS/HTML
- Normalize server_key to "default" early in verify/VLAN-override views
- DoesNotExist guard on select_for_update in CreateAndAssignPlatformView
- Comprehensive tests: test_librenms_id, test_mixins, test_sync_devices,
test_sync_interfaces, test_sync_view_mismatch, test_permissions,
test_view_wiring smoke tests for new views
…_key in sync redirects SingleCableVerifyView.post() now strips derived fields from cached link data and re-enriches remote side from current NetBox state, preventing DoesNotExist when remote devices/interfaces are deleted after caching. Sync redirect URLs for interfaces, cables, IP addresses, and VLANs now preserve the server_key query parameter so users return to the correct multi-server tab.
…cy display
- Reject boolean inputs in _librenms_id_q to prevent false matches on
ID 1/0.
- Add response.ok check to handleVRFChange JS fetch (consistency with
other endpoints).
- Unify view.request binding in test_import_utils.py so permission
checks and business logic use the same request object.
- Support string-digit legacy IDs in migrate_legacy_librenms_id
(e.g. "42" → {"server": 42}).
- Fix btn_title/visual mismatch in device_status.py when
name_sync and migration are both true.
- Guard get_librenms_device_id dict branch to only return int values
(reject floats, lists, dicts).
- Clamp cache_ttl > 0 in all 5 sync views to prevent bogus countdown
from negative TTLs.
- Make _build_sync_info VM-safe: use getattr for serial/device_type.
- Legacy display_name fallback in _build_id_server_info for
single-server installs.
- Remove reflected XSS in device_fields.py error responses.
- utils.py: remove duplicate docstring sentences in get_librenms_device_id - views/object_sync/devices.py: remove erroneous server_key kwarg from VCInterfaceTable/LibreNMSInterfaceTable constructor call - views/object_sync/vms.py: remove redundant get_vlan_context override (base class already returns None for VMs) - views/imports/actions.py: remove overly strict migration gate that blocked valid migrate_librenms_id POSTs when validation flag was missing - jobs.py: persist use_sysname and strip_domain in FilterDevicesJob.data so result loading can reconstruct naming preferences - templates/librenms_sync_base.html: remove spurious blank line - views/base/cables_view.py: remove class-level interface_name_field that called get_interface_name_field() at import time; resolve per-request instead; guard against None port_id and port_name in local_ports_map build - views/base/interfaces_view.py: thread server_key through get_context_data() so post() passes the freshly-resolved key rather than re-resolving from API - views/base/ip_addresses_view.py: analogous server_key threading fix - views/base/librenms_sync_view.py: analogous improvements - views/imports/list.py: miscellaneous improvements - forms.py: improvements - import_utils/: various correctness improvements across bulk_import, cache, device_operations, filters, virtual_chassis, vm_operations
- tests/mock_librenms_server.py: new mock server helper for integration tests - tests/test_vm_operations.py: new — comprehensive VM import and operations tests (652 lines) covering VM import, VC detection, validation, permissions - tests/test_integration_sync.py: new — integration-level sync tests (346 lines) covering interface, cable, IP address, and VLAN sync end-to-end flows - tests/test_import_utils.py: major expansion covering import validation, bulk import, VM import, VC detection, caching, and error paths - tests/test_librenms_id.py: additional edge-case coverage - tests/test_permissions.py: additional permission combinations - tests/test_background_jobs.py: additional job lifecycle tests - tests/test_sync_interfaces.py: additional interface sync assertions - tests/test_utils.py: additional utility function coverage
…locked-row recheck, aria-labels, tests Code fixes: - cables.py: escape member.name in render_device_selection (XSS) - librenms_sync.js: add server_key to handleCableChange, guard radio lookup, close VLAN modal only on success - utils.py: boolean guard in find_by_librenms_id, auto_save=False skips mutation - device_fields.py: bool/strict-digit in _normalize_librenms_mapping, recheck locked row preconditions, getattr for VM serial - ip_addresses_view.py: auto_save=False in prefetch, id on template hidden input - librenms_sync_view.py: validate did in server mapping loop - virtual_chassis.py: validate _load_vc_member_name_pattern return - vm_operations.py: reject boolean device_id - device_status.py: aria-labels on icon-only buttons - device_validation_details.html: use resolved_name Tests: - test_librenms_id.py: auto_save mutation tests, find_by_librenms_id bool guard - test_vm_operations.py: boolean device_id rejection - test_reviewer_fixes.py: _load_vc_member_name_pattern, _normalize_librenms_mapping, _build_all_server_mappings did validation, render_device_selection XSS
- virtual_chassis.py: don't cache None from detect_virtual_chassis_from_inventory
to allow retry on transient API failures
- librenms_api.py: remove 6 redundant status_code==200 checks after raise_for_status()
in get_device_ips, get_device_inventory, get_poller_groups, get_inventory_filtered,
list_devices, get_device_vlans
- import_utils/filters.py: use (d.get('type') or '').lower() and (d.get('os') or '').lower()
to guard against explicit None values from LibreNMS
- tests/test_coverage_api.py: fix non-200 tests to use HTTPError side_effect
- tests/test_import_utils.py: fix cache None test to assert_not_called
- tests/test_coverage_list.py: new 40-test file for views/imports/list.py (100% coverage)
- pyproject.toml: add coverage.run omit to exclude test files from coverage
- docs/development/testing.md: update with new test files, timing, coverage notes
- filters.py: guard hostname/sysName against explicit None values using (val or "") pattern - bulk_import.py: tighten librenms_id truthiness check to skip None/bool values only - librenms_sync_view.py: use friendly label for legacy single-server default display name - device_operations.py: remove redundant rack re-assignment after manual_mappings block - vm_operations.py: use validation.resolved_name as vm_name fallback before recomputing - vm_operations.py: add cancellation check at idx==1 to catch cancellation before first VM - test_coverage_api.py: assert set_librenms_device_id with explicit server_key instead of proxy save - test_coverage_device_operations.py: tighten serial conflict assertion (remove or-guard) - test_coverage_filters.py: add non-matching device to client-filter fixture for real exclusion proof - test_coverage_sync_view.py: tighten _strip_vc_pattern assertion to exact value - test_coverage_virtual_chassis.py: tighten VC position assertions to exact sorted sequences - test_sync_view_mismatch.py: add VC lookup delegation tests for both scenarios - test_vm_operations.py: update cancellation assertions to reflect idx==1 early check
Added a pull request template to standardize PR submissions.
Code fixes: - librenms_api.py: get_librenms_id() passes auto_save=False to stay read-only (Thread 36) - librenms_api.py: guard addresses key in get_device_ips() response (Thread 37) - virtual_chassis.py: prefer stack over chassis in VC detection (Thread 34) - virtual_chassis.py: add dict|None and VirtualChassis return type annotations (Threads 46,47) - bulk_import.py: remove interface/VC permissions from initial check (Thread 49) - bulk_import.py: pass pre-computed validation to import_single_device (Thread 50) - pyproject.toml: fix misleading C901 complexity comment (Thread 42) Test fixes: - test_coverage_api.py: assert auto_save=False in get_librenms_id call (Thread 36) - test_coverage_filters.py: add server-key cache isolation test (Thread 38) - test_coverage_filters.py: tighten status branch tests (Thread 39) - test_coverage_virtual_chassis.py: assert specific serial+position pairs (Thread 41) - test_coverage_sync_view.py: update VC-member GET tests with two cases (Thread 48) - test_permissions.py: update bulk import permission assertions after Fix 6
… not gated by VC flag
…ntory_filtered; read VM role from validation
…rint, double VC refresh, stale device_role, VC master position, strip_domain threading, test improvements
…e_role schema, test class dedup
Added a pull request template to standardize PR submissions.
…rphaned section header
…ck server helpers - get_virtual_chassis_data() now caches empty_virtual_chassis_data() when detect_virtual_chassis_from_inventory() returns None (single device or transient API failure), so prefetch_vc_data_for_devices() eliminates repeated hits for non-stack devices - force_refresh=True remains the manual bypass - Update test_cache_miss_detect_returns_none_stores_empty to assert set() is called and cached value is empty (was asserting set() not called) - Sync mock_librenms_server.py with VC helpers (inventory_response, vc_inventory_callable) from inventory-rebased - Add test_integration_virtual_chassis.py with 24 integration tests covering full detection pipeline, caching, prefetch, and port fetch
…mport Users with device import rights but without dcim.add_virtualchassis could silently create a VirtualChassis. Add a check_user_permissions() guard right before the VC creation block; on failure log a warning and skip VC creation while allowing the device import to succeed. Closes #31
…date_fields, Q string normalization, mapping fail-closed, test assertions, docs
marcinpsk
force-pushed
the
pr/librems-id-multi-fixes
branch
from
March 9, 2026 19:59
88b225d to
11e6795
Compare
…rms to bulk import gate
- device_operations.py: guard cluster-required and role-required blockers with
'if not result.get("existing_device"):' so existing VM/device matches don't
get bogus create-time prerequisites appended (fixes CodeRabbit #2907400959)
- bulk_import.py: add virtualization.add/change_virtualmachine to the upfront
permission check in bulk_import_devices_shared(), since any device may be
flagged as import_as_vm during validation (fixes CodeRabbit #2907400951)
…ey in job data, harden get_object fallback, fix existing device URL derivation - bulk_import_devices_shared() and bulk_import_devices() now accept vc_detection_enabled (default False) and pass it to validate_device_for_import() as include_vc_detection so import respects the same VC flag used during the filter/preview step - ImportDevicesJob.run() accepts and forwards vc_detection_enabled; BulkImportDevicesView.post() extracts the flag from POST data - jobs.py: store api.server_key (resolved) not the raw server_key argument (which may be None) so _load_job_results cache key lookups always succeed with a concrete key - sync/devices.py AddDeviceToLibreNMSView.get_object(): wrap VM-first lookup in get_object_or_404 to return 404 instead of 500 on missing object - device_validation_details.html: derive existing_device_url from existing_device_model_name (the actual matched model) instead of import_as_vm flag to correctly link VMs without a cluster and Devices that were matched for a VM-flagged import - list.py _load_job_results(): mirror use_sysname/strip_domain from job data onto self so toggle state matches cached results - test_background_jobs.py: set mock_api.server_key on mocks that assert job.data["server_key"] to reflect resolved-key storage
Modules tab bug: - BaseLibreNMSSyncView.get_context_data() never called get_module_context() and never added module_sync to context; template guard hid the tab silently. Fix: call get_module_context(request, obj), add module_sync to context dict, add default get_module_context() returning None to base class. Issue #27: - Remove 'existing_device_model_name != virtualmachine' guard from sync_name button in device_validation_details.html. DeviceConflictActionView supports sync_name for VMs.
- cache.py: make server_key required in get_import_device_cache_key - filters.py: add negative caching when LibreNMS API call fails - vm_operations.py: hard-fail instead of warning when user-selected cluster/role no longer exists - test_import_utils.py: pass explicit server_key to match new signature
marcinpsk
added a commit
that referenced
this pull request
Mar 28, 2026
- #21: get_device_info uses raise_for_status() instead of manual status_code == 200 check, consistent with all other API methods - #22: NormalizationRule.manufacturer FK on_delete changed CASCADE -> SET_NULL so deleting a Manufacturer orphans rules (becomes global) rather than destroying admin-created normalization rules; migration 0012 - #23: getDeviceIdFromUrl() fallback branches now validate extracted path segment is numeric (/^\d+$/.test) before returning - #24: setInterfaceNameFieldFromURL() allowlists interface_name_field to ['ifDescr', 'ifName'] before interpolating into querySelector template literal (CSS selector injection prevention) - #25: Delete orphaned inc/_module_sync.html (near-duplicate differing only in server_key variable name; included by nothing) - #26: Remove unused interfaceNames variable in delete-interfaces handler - #27: Replace mark_safe with format_html in tables/mappings.py render_pattern and render_require_serial_match_parent; remove unused mark_safe import - #28: Use lazy %s formatting in logger calls in api/views.py instead of f-strings - #30: Move inline 'import re' inside forms.py method to module level
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
Follow-up to PR #20 (multi-server librenms_id). Addresses reviewer findings, fixes code correctness issues discovered during the refactor, and significantly expands test coverage.
Depends on PR #20 (
pr/librenms-id-multi-server) — merge that first.Full stack: PR #23 → PR #20 → this PR.
Motivation / Problem
Code review of PR #20 surfaced additional correctness issues and missing test coverage for edge cases in the multi-server feature.
Scope of Change
How Was This Tested?
Security & correctness fixes (delta from PR #20)
get_librenms_device_id(auto_save=False)no longer mutatesobj.custom_field_dataon read-only paths (e.g._prefetch_netbox_data)select_for_update()inConvertLegacyLibreNMSIdView.post()find_by_librenms_id(),_normalize_librenms_mapping(),_build_all_server_mappings(),create_vm_from_librenms()validate_device_for_import()resolvesapi.server_key;handleCableChangeJS sendsserver_key; IP sync template getsid="current-server-key"getattr(obj, "serial", "")in device_fields.py; guard radio lookup inhandleInterfaceChangeuse_sysname/strip_domaininFilterDevicesJob.data; resolveinterface_name_fieldper-requestCode correctness
server_keykwarg from table constructorsget_vlan_contextoverride in VM sync viewmigrate_librenms_idPOSTsserver_keythroughget_context_data()in interfaces/IP/cables viewsport_id/port_nameinlocal_ports_mapbuildresolved_namein device validation details template.isdigit()checks in_normalize_librenms_mappingfor string-to-int coercionTest coverage expansion
test_vm_operations.py— VM import, VC detection, validation, permissionstest_integration_sync.py— interface, cable, IP, VLAN sync end-to-endmock_librenms_server.py— reusable mock server helper for integration teststest_librenms_id.py,test_import_utils.py,test_permissions.py,test_background_jobs.py,test_sync_interfaces.py,test_utils.pyDocs
testing.mdwith new test files and conventionsRisk Assessment
auto_save=Falsefix prevents unintended writes that the previous code could cause.Low risk. All changes are additive safety improvements or test coverage.
Backwards Compatibility
Other Notes
bool-is-subclass-of-intissue (isinstance(True, int) == True) required guards in ~10 places. All covered by regression tests.ConvertLegacyLibreNMSIdViewprevents a TOCTOU race where another request could modify the device between the initial read and theselect_for_update()lock.