Conversation
Test/ci workflow
Feature/snmpv1-support
- Remove trailing whitespace from templates, JS, docs, and config files - Add missing newlines at end of files - Applied via pre-commit hooks (first run after installation)
…ess control - Add LibreNMSPermissionMixin and NetBoxObjectPermissionMixin to all views - Enforce view/change permissions on LibreNMSSettings model - Add NetBox object permission checks on sync POST handlers - Auto-fallback to synchronous mode for non-superuser background jobs - Update navigation, API, templates, and import UI for permission gating - Add user-facing permissions documentation and mkdocs nav entry - Add test_permissions.py and update test_background_jobs.py
Updated lint and format workflow to updated action v4 and improved output formatting.
…custom field Devcontainer & CI: - Add proxy/CA bundle support with ALLOW_GIT_SSL_DISABLE opt-in - Add Codespaces configuration loader - Remove unnecessary proxy env vars from postgres/redis services - Extract detect_plugin_workspace() helper, idempotent .bashrc guard - Consolidate aliases into load-aliases.sh as single source of truth - Fix CI test workflow to run from correct NetBox directory - Add media/configuration.testing.py for CI - Update lint workflow: actions v4/v5, Python 3.12, fail on lint errors - Exclude tests from package distribution - Fix MD031 markdown lint in README - Add security note about embedding proxy credentials in URLs Plugin: - Auto-create librenms_id custom field via post_migrate signal - Log exceptions instead of silently swallowing them in custom field creation - Add inline comments on _executed flag lifecycle assumptions - Raise KeyError for non-default missing server keys in LibreNMSAPI Tests: - Add setup_method for consistent _executed flag reset - Assert exception logging in test_exception_does_not_propagate - Fix fragile getLogger assertion in test_no_log_when_field_already_exists
feat: devcontainer proxy support, auto-create librenms_id …
…ructions chore: update copilot instructions with permission and background job patterns
Validate Referer header with url_has_allowed_host_and_scheme before using it for HX-Redirect or redirect targets. Falls back to request.path when the referrer is external or missing.
Add dcim.add_virtualchassis to bulk import permission checks. Explicitly validate object_type in sync views, raising Http404 for invalid values instead of silently defaulting to VM permissions.
Replace inline HTML alert response with messages.error and HX-Redirect to match the permission denial pattern used elsewhere.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR implements a comprehensive two-tier permission system integrating plugin-level and NetBox object-level permissions. It adds serial-based device conflict resolution with per-field sync actions, expands SNMP support from v2c-only to v1/v2c/v3, persists user UI preferences, and enhances device validation workflows with detailed conflict handling and HTMX-driven actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant View as DeviceValidationDetailsView
participant ImportUtils as import_utils
participant Backend as NetBox Backend
participant LibreNMS as LibreNMS API
User->>View: POST conflict-action (e.g., sync_name)
View->>View: require_all_permissions()
View->>View: validate request parameters
View->>ImportUtils: check_user_permissions()
ImportUtils-->>View: permission check result
alt Insufficient Permissions
View-->>User: PermissionDenied / redirect
else Permissions OK
View->>LibreNMS: fetch device by librenms_id
View->>Backend: get existing_device
View->>View: _build_sync_info(libre_device, existing)
View->>View: apply action (update fields, validate)
View->>Backend: full_clean() and save()
View->>View: log action
View-->>User: return updated row / success message
end
sequenceDiagram
participant Client as Browser
participant View as BulkImportDevicesView
participant ImportUtils as import_utils
participant NetBox as NetBox ORM
participant LibreNMS as LibreNMS API
Client->>View: POST bulk_import_devices (device_ids)
View->>View: require_all_permissions()
View->>ImportUtils: bulk_import_devices_shared(device_ids, user)
ImportUtils->>ImportUtils: require_permissions(user, permissions)
alt User lacks permissions
ImportUtils-->>View: PermissionDenied raised
View-->>Client: 403 / redirect with error message
else Permissions granted
ImportUtils->>LibreNMS: fetch device data per device_id
ImportUtils->>NetBox: validate each device (site, type, serial)
ImportUtils->>NetBox: check existing device by librenms_id/hostname/serial
ImportUtils->>ImportUtils: _refresh_existing_device() if match found
ImportUtils->>NetBox: create or update device
ImportUtils-->>View: return import results
View->>Client: render result table / confirmation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @.devcontainer/scripts/setup.sh:
- Around line 54-64: Before running the csplit and renaming loop, remove any
existing split fragments in the proxy cert directory so stale cert-* and
cert-*.crt files don't accumulate; update the setup block that contains mkdir -p
/usr/local/share/ca-certificates/proxy, the csplit invocation, and the for f in
/usr/local/share/ca-certificates/proxy/cert-* loop to first delete existing
cert-* and cert-*.crt files (use a safe rm or find -delete) so the directory is
clean before splitting the new "$CA_BUNDLE_SRC".
In `@netbox_librenms_plugin/import_utils.py`:
- Around line 798-812: The code sets result["serial_action"]="update_serial"
even when a serial_conflict is found; change the logic in the incoming_serial
check (using variables incoming_serial, existing_device, serial_conflict) so
that when serial_conflict is truthy you do NOT set "update_serial" — instead set
a blocking action like result["serial_action"]="conflict" (or leave it
unset/None) and keep the serial_conflict warning; only set
result["serial_action"]="update_serial" in the else branch where no
serial_conflict exists.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 728-741: The CSRF token retrieval for the save-preference POST can
fail when HTMX swaps remove the hidden input; update the fetch preparation in
the block that uses savePrefUrl (and htmx.config.defaultHeaders) to fall back to
calling getCookie('csrftoken') when
document.querySelector('[name=csrfmiddlewaretoken]') returns null/empty, and
pass that value as the X-CSRFToken header; ensure the existing getCookie
function is referenced and used to avoid 403s from missing the
csrfmiddlewaretoken input.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 262-277: The branch that renders the "Sync platform" form can
render an empty existing_device_id when validation.existing_device is None;
update the template so the form and its hidden input <input
name="existing_device_id"> are only rendered when validation.existing_device is
truthy (e.g. wrap the form and its inputs with an explicit check like if
validation.existing_device), otherwise render the "Not set" text or a
disabled/absent action button; reference the template symbols
validation.existing_device, validation.existing_device.platform,
sync_info.platform_info.platform_exists, the hidden existing_device_id input,
and the hx-post URL to device_conflict_action using libre_device.device_id to
locate the code to change.
- Around line 318-333: The Primary IP row shows the check icon before the
LibreNMS IP but after the existing_device primary IP; update the template logic
so both branches render the IP followed by the check icon for consistency: in
the branch that checks libre_device.ip (using variable libre_device.ip) swap the
order so you output {{ libre_device.ip }} first and then the <span
class="text-success"><i class="mdi mdi-check-circle"></i></span>; ensure the
existing_device branch (validation.existing_device.primary_ip) remains IP then
icon and keep the fallback "No primary IP" unchanged.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 724-735: The comparison assumes existing_device.device_type exists
and can raise AttributeError; in the device type sync block (variables
device_type_synced, librenms_device_type and the call
match_librenms_hardware_to_device_type), first check whether
existing_device.device_type is truthy (not None) before accessing .pk; if
existing_device.device_type is missing and hw_match.get("matched") is true, set
device_type_synced = False and assign librenms_device_type appropriately so the
flow treats it as out-of-sync instead of crashing.
- Around line 704-716: Replace the direct ORM lookup of
Platform.objects.get(...) with the shared exact-only helper
find_matching_platform from utils.py: call find_matching_platform(librenms_os)
(or pass the same librenms_os value used currently) to determine whether a
matching platform exists and to retrieve the matching Platform object, then set
platform_info["platform_exists"] and platform_info["matching_platform"] based on
that result; apply the same replacement in the other occurrence that builds
platform_info (the block referenced around the second region) so both the sync
action and the validation summary use find_matching_platform instead of direct
queries.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 10-39: The post handler in UpdateDeviceNameView currently assigns
sys_name to device.name and calls device.save() without handling model
validation or DB uniqueness errors; wrap the update in a try/except around
device.full_clean() and device.save() (or just device.save() if you prefer)
catching Django ValidationError and IntegrityError, restore device.name to
old_name on failure, and call messages.error(request, ...) with the caught error
message before redirecting back to the sync view; reference
UpdateDeviceNameView.post, the sys_name variable,
device.full_clean()/device.save(), and messages.error to locate where to add the
try/except and redirect logic.
feat: add two-tier permission system with plugin and object-level access control
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@netbox_librenms_plugin/import_utils.py`:
- Around line 2090-2112: The _refresh_existing_device function currently
swallows all exceptions with a bare except; replace that with catching Exception
as e and log the error (including context like validation.get("existing_device")
and existing.pk or validation keys) instead of silently passing so
DB/permission/connectivity failures are visible; use the module logger (e.g.
logging.getLogger(__name__) or the project's existing logger) inside the except
block and keep the existing behavior of setting
validation["existing_device"]=None only when refreshed is falsy.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 732-741: The fetch call saving user prefs (using savePrefUrl and
csrfToken via getCookie) is fire-and-forget and may send a string "null" or
produce unhandled rejections; update the block so you first validate csrfToken
(if it's falsy, bail out or skip the fetch) and then attach a `.catch()` to the
fetch promise to swallow/log errors (e.g., console.debug/console.error) so
failures don't raise unhandled rejections; target the existing savePrefUrl
lookup and the fetch(...) invocation that posts {key: 'interface_name_field',
value: this.value}.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 95-106: Change the inline HTMX forms (e.g., the form that posts to
the device_conflict_action URL) to use hx-swap="none" and remove the
hx-target="#device-row-…" attribute (you can keep hx-include selectors), and
update DeviceConflictActionView (and its helper render_device_row) to return the
updated table row as a <tr> element with hx-swap-oob="true" and the existing id
("device-row-{{ libre_device.device_id }}") so the row is replaced out-of-band
instead of using outerHTML swaps.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 884-892: The backend handler for the "sync_serial" action (the
branch using variables action, libre_device, incoming_serial and
existing_device) must enforce ownership checks before assigning serials: after
computing incoming_serial but before setting existing_device.serial and calling
existing_device.save(), query the device model for any other device that already
has that serial (exclude existing_device by PK) and if one exists return an HTTP
409 (or 400) indicating a serial conflict; only assign and save when no other
device owns the incoming_serial. Ensure you keep the existing checks for
empty/"-" serials and use the same logger to record successful syncs or conflict
responses.
- Around line 822-872: Multiple action branches (link, update, update_serial,
sync_name, etc.) call existing_device.save() directly after mutating fields
(e.g., name, serial, device_type) which can raise ValidationError or
IntegrityError; update each branch that mutates existing_device (link, update,
update_serial, update_type, sync_serial, sync_platform, sync_device_type,
sync_name) to call existing_device.full_clean() before saving and wrap
existing_device.save() in a try/except that catches ValidationError and
IntegrityError, logs a clear message including existing_device.pk and
librenms_id, and returns/raises a user-friendly error response consistent with
the view’s existing error handling pattern; locate mutations via symbols
_determine_device_name, existing_device.custom_field_data, existing_device.name,
existing_device.serial, existing_device.device_type to apply the pattern
uniformly.
- Around line 940-963: The SaveUserPrefView.post currently allows anonymous
access and relies on _save_user_pref to silently ignore unauthenticated users;
make authentication explicit by having SaveUserPrefView inherit from
LoginRequiredMixin (so the view requires login) and ensure post validates the
incoming value type before calling _save_user_pref (e.g., check key from
ALLOWED_PREFS and coerce/validate value consistent with expected types for that
preference) to avoid relying on implicit handling inside _save_user_pref.
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 (10)
netbox_librenms_plugin/views/sync/locations.py (2)
115-126:⚠️ Potential issue | 🟠 Major
create_librenms_locationcan send"None"as coordinates.
build_location_data(line 160) callsstr(site.latitude)/str(site.longitude)unconditionally.update_librenms_locationguards againstNonelat/lng on line 130, butcreate_librenms_locationdoes not—so a site with no coordinates will postlat="None",lng="None"to the LibreNMS API.Proposed fix
def create_librenms_location(self, request, site): """Create a new location in LibreNMS from the given site.""" + if site.latitude is None or site.longitude is None: + messages.warning( + request, + f"Latitude and/or longitude is missing. Cannot create location '{site.name}' in LibreNMS.", + ) + return redirect("plugins:netbox_librenms_plugin:site_location_sync") location_data = self.build_location_data(site)
46-53: 🧹 Nitpick | 🔵 TrivialLines 49-51 are unreachable dead code.
self.filtersetis assigned the classSiteLocationFilterSetat line 19, so it is always truthy. When any GET parameters are present (self.request.GETis truthy), line 47 always returns, making the manualq-filtering on lines 49-51 unreachable.Remove dead code
if self.request.GET and self.filterset: return self.filterset(self.request.GET, queryset=sync_data).qs - if "q" in self.request.GET: - query = self.request.GET.get("q", "").lower() - sync_data = [item for item in sync_data if query in item.netbox_site.name.lower()] - return sync_datanetbox_librenms_plugin/views/sync/interfaces.py (1)
252-261:⚠️ Potential issue | 🟡 Minor
interface_namemay be unbound if an exception is raised before line 252.If a non-
DoesNotExistexception occurs duringInterface.objects.get(...)at line 233 or during the ownership checks (lines 234–250),interface_nameis never assigned (it's set at line 252). Theexcept Exceptionhandler at line 259 then referencesinterface_name, which will raise anUnboundLocalErroron the first loop iteration (or leak a stale name from a prior iteration).🐛 Proposed fix
try: + interface_name = f"ID {interface_id}" if object_type == "device": interface = Interface.objects.get(id=interface_id)This provides a safe default before any lookup, so the broad
exceptalways has a usable name.netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (2)
269-276: 🧹 Nitpick | 🔵 Trivial
hideModalcreates a throwawayModalManager, bypassing any existing Bootstrap instance.
hideModal(andshowModal) each instantiate a freshModalManagerwhosethis.instanceis alwaysnull. Thehide()method therefore never takes the Bootstrap path (line 93) and always falls through to_hideManual(). If the modal was originally shown via a BootstrapModalinstance (e.g., fromshowModalor another caller), that instance is never properly disposed, which can leave stale event listeners or state.This currently works because the environment uses Tabler without
bootstrap.Modalhelpers (per project guidelines), so the manual path is always taken. However, it's fragile — if Bootstrap modals become available, the show/hide asymmetry will surface as bugs (e.g., orphaned backdrops).Consider either:
- Storing a shared
ModalManagerper modal element (likefilterModalManagerat line 630), or- Having
ModalManager.hide()also trybootstrap.Modal.getInstance(this.modal)before falling back to manual.Option 2: make hide() resilient to throwaway instances
hide() { if (!this.modal) return false; // Try Bootstrap instance - if (this.instance && typeof this.instance.hide === 'function') { - this.instance.hide(); - this.isVisible = false; - return true; + if (typeof bootstrap !== 'undefined' && bootstrap.Modal) { + const existing = bootstrap.Modal.getInstance(this.modal); + if (existing) { + existing.hide(); + this.isVisible = false; + return true; + } } // Fallback: Manual
360-365: 🧹 Nitpick | 🔵 TrivialRepetitive
new ModalManager(modal).hide()pattern insidepollJobStatus.The modal
#filter-processing-modalis looked up and wrapped in a new throwawayModalManagerat ~10 locations throughoutpollJobStatus. This duplicates the DOM query, creates unnecessary objects, and (as noted above) always bypasses the Bootstrap path.Consider accepting a
ModalManagerparameter (the existingfilterModalManager) or creating one at the top ofpollJobStatusand reusing it.Sketch: pass the manager or create once
-function pollJobStatus(jobId, jobPk, pollUrl, baseUrl, originalFilters, deviceCount) { +function pollJobStatus(jobId, jobPk, pollUrl, baseUrl, originalFilters, deviceCount, modalManager) { + // Use provided manager, or create one from the known modal ID + const processingModal = modalManager || (() => { + const el = document.getElementById('filter-processing-modal'); + return el ? new ModalManager(el) : null; + })(); ... - const modal = document.getElementById('filter-processing-modal'); - if (modal) { - const manager = new ModalManager(modal); - manager.hide(); - } + if (processingModal) processingModal.hide();Also applies to: 407-411, 428-432, 445-449, 464-468, 549-553
netbox_librenms_plugin/tables/cables.py (1)
118-137: 🧹 Nitpick | 🔵 TrivialStabilize VC member ordering in the dropdown.
members.all()can return an arbitrary order; sorting by VC position keeps the UI deterministic.♻️ Suggested tweak
- members = self.device.virtual_chassis.members.all() + members = self.device.virtual_chassis.members.order_by("vc_position", "name")netbox_librenms_plugin/import_utils.py (1)
2093-2283:⚠️ Potential issue | 🟠 MajorRecompute readiness when a cached existing device disappears.
_refresh_existing_device()can setexisting_devicetoNonebut leavesexisting_match_type,can_import, andis_readyuntouched. This can wrongly block imports until cache expiry. Consider marking the validation as stale and re-runningvalidate_device_for_import()for that device (or recomputing readiness fields) when a previously linked device is no longer present.🔧 Example approach
-def _refresh_existing_device(validation: dict) -> None: +def _refresh_existing_device(validation: dict) -> bool: ... if refreshed: ... - return + return True else: validation["existing_device"] = None + validation["existing_match_type"] = None + return False- _refresh_existing_device(device["_validation"]) + if not _refresh_existing_device(device["_validation"]): + validation = validate_device_for_import( + device, api=api_for_validation, include_vc_detection=vc_detection_enabled, force_vc_refresh=clear_cache + ) + device["_validation"] = validationnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html (1)
266-271:⚠️ Potential issue | 🟡 MinorHTML tag mismatch:
<td>opened but closed with</th>.Line 266 opens a
<td>element but Line 271 closes it with</th>. While browsers may auto-correct this, it's invalid HTML and could cause rendering inconsistencies.🔧 Proposed fix
- </th> + </td>netbox_librenms_plugin/views/sync/device_fields.py (2)
76-78: 🛠️ Refactor suggestion | 🟠 Major
UpdateDeviceSerialView.save()lacks the same validation guard asUpdateDeviceNameView.
UpdateDeviceNameViewnow wrapssave()infull_clean()+try/except, butUpdateDeviceSerialView(and the other sibling views) still calldevice.save()without validation. Serial numbers could also trigger uniqueness or validation errors. Consider applying the same pattern for consistency and robustness.♻️ Suggested pattern (same as UpdateDeviceNameView)
old_serial = device.serial device.serial = serial - device.save() + try: + device.full_clean() + device.save() + except (ValidationError, IntegrityError) as e: + device.serial = old_serial + error_msg = e.message_dict if hasattr(e, "message_dict") else str(e) + messages.error(request, f"Failed to update serial to '{serial}': {error_msg}") + return redirect("plugins:netbox_librenms_plugin:device_librenms_sync", pk=pk)
217-220:⚠️ Potential issue | 🟡 Minor
Platform.objects.createcan raiseIntegrityErroron slug collision.If two platforms with names that produce the same slug are created concurrently,
Platform.objects.createwill raise an unhandledIntegrityErrorand return a 500. Wrap in a try/except or useget_or_create.
🤖 Fix all issues with AI agents
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 87-112: The Name link (and other existing-device links) uses
dcim:device even when validation.existing_device is a VM, causing 404s; update
the template to compute a single existing_device_url variable based on whether
validation.import_as_vm or validation.existing_device.cluster is truthy (use the
virtualization:virtualmachine URL when true, otherwise dcim:device), e.g. with
{% url ... as existing_device_url %}, then replace the hard-coded {% url
'dcim:device' pk=validation.existing_device.pk %} occurrences (including the
Name row and the status banners) to use href="{{ existing_device_url }}" so all
existing-device links point to the correct resource.
- Around line 526-548: The Close button currently uses Bootstrap's
data-bs-dismiss attribute; remove data-bs-dismiss="modal" from the button in
device_validation_details.html and instead wire it to the HTMX modal close
mechanism used by librenms_import.html by adding the same attributes/event used
to target the htmx-modal-content wrapper (e.g., the closeModal trigger or the
htmx-target/htmx-swap attributes used elsewhere), ensuring it doesn't
reintroduce data-bs-toggle or duplicate modal IDs and that it targets the
htmx-modal-content element when validation.existing_device rendering reaches the
modal-footer block.
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 1107-1155: Tests in TestSerialNumberMatching repeat the same nine
`@patch` decorators across methods which is verbose; refactor by moving common
patches into a class-level setup or a pytest fixture: consolidate the nine mocks
(patches for VirtualMachine, Device, find_matching_site, find_matching_platform,
match_librenms_hardware_to_device_type, DeviceRole, Cluster, Rack, Site) into a
shared setup method (e.g., setUpClass / setup_method) or a module/class fixture
and adjust test_serial_match_blocks_import to use the shared mock attributes
(including the device_filter side_effect and
mock_vm.objects.filter.return_value.first.return_value) so you remove the
repeated decorators while preserving the same behavior and references to
validate_device_for_import, device_filter, mock_device, mock_vm, and existing.
In `@netbox_librenms_plugin/views/object_sync/devices.py`:
- Around line 63-65: The get_redirect_url method in DeviceInterfaceTableView is
reversing the wrong route; update the reverse call in get_redirect_url (in
devices.py) to use "device_interface_sync" instead of "vm_interface_sync" so it
points to devices/<int:pk>/interface-sync/ while keeping kwargs={"pk": obj.pk}
intact.
In `@netbox_librenms_plugin/views/settings_views.py`:
- Line 10: Rename the underscored helper _save_user_pref in
netbox_librenms_plugin.utils to a public name save_user_pref and update all
imports and call sites to use save_user_pref (for example update the import in
settings_views.py and any usages in views/imports/); ensure the function
definition in utils is renamed and any internal references adjusted so
tests/imports resolve correctly and remove the leading underscore from both the
export and all import statements.
- Around line 69-79: The two calls to _save_user_pref (saving
"plugins.netbox_librenms_plugin.use_sysname" and
"plugins.netbox_librenms_plugin.strip_domain") should be wrapped in a try/except
so failures updating per-user prefs don't raise and interrupt the
already-committed settings save; add logger = logging.getLogger(__name__) at the
top of the file, then surround the _save_user_pref calls with a try: block and
except Exception as e: that logs a warning (via logger.warning) including the
exception details and the user/request context and then continues to the normal
redirect flow.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Line 136: The update_interface_attributes function currently calls
interface.save(), causing a redundant second DB write because sync_interface
also saves; remove the interface.save() call from update_interface_attributes
and ensure sync_interface performs the single save once after all mutations
(including setting enabled) so all changes are persisted in one DB write.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
netbox_librenms_plugin/views/settings_views.py (1)
136-179:⚠️ Potential issue | 🟠 MajorXSS: external data interpolated into HTML without escaping.
Values like
version,database,php_version,error_msg, andstr(e)are injected directly into HTML via f-strings. Since these originate from an external LibreNMS API response or exception messages, a compromised or misconfigured server could inject arbitrary HTML/JS.Use
django.utils.html.escape()on all interpolated values, or render via a Django template (which auto-escapes by default).Proposed fix (escape approach)
+from django.utils.html import escape + # In the success branch (around line 137): - version = system_info.get("local_ver", "Unknown") - database = system_info.get("database_ver", "Unknown") - php_version = system_info.get("php_ver", "Unknown") + version = escape(system_info.get("local_ver", "Unknown")) + database = escape(system_info.get("database_ver", "Unknown")) + php_version = escape(system_info.get("php_ver", "Unknown")) # In the error branch (around line 151): - error_msg = system_info.get("message", "Unknown error occurred") + error_msg = escape(system_info.get("message", "Unknown error occurred")) # In the except blocks (lines 168, 174): - f"<strong>Configuration error:</strong><br>{str(e)}" + f"<strong>Configuration error:</strong><br>{escape(str(e))}" - f"<strong>Connection failed:</strong><br>{str(e)}" + f"<strong>Connection failed:</strong><br>{escape(str(e))}"netbox_librenms_plugin/tables/cables.py (1)
122-137: 🧹 Nitpick | 🔵 TrivialConsider consistent member ordering across tables.
VCCableTable.render_device_selectionorders members with.order_by("vc_position", "name")(line 124), but the equivalent method innetbox_librenms_plugin/tables/interfaces.py(line 330) usesmembers.all()without explicit ordering. Consider aligning both for a consistent member dropdown order.#!/bin/bash # Check if the interface table's render_device_selection uses ordering rg -n "render_device_selection" netbox_librenms_plugin/tables/interfaces.py -A 10netbox_librenms_plugin/views/sync/device_fields.py (2)
131-134:⚠️ Potential issue | 🟠 MajorInconsistent error handling:
device.save()withoutfull_clean()or try/except.
UpdateDeviceTypeViewandUpdateDevicePlatformViewboth calldevice.save()without thefull_clean()+ try/except guard that was added to name and serial updates. If saving raises aValidationErrororIntegrityError, it will result in an unhandled 500 error.🛠️ Suggested fix for UpdateDeviceTypeView
device_type = match_result["device_type"] old_device_type = device.device_type device.device_type = device_type - device.save() + try: + device.full_clean() + device.save() + except (ValidationError, IntegrityError) as e: + device.device_type = old_device_type + error_msg = e.message_dict if hasattr(e, "message_dict") else str(e) + messages.error(request, f"Failed to update device type: {error_msg}") + return redirect("plugins:netbox_librenms_plugin:device_librenms_sync", pk=pk)Apply the same pattern to
UpdateDevicePlatformView(line 183) andCreateAndAssignPlatformView(line 237).
270-288: 🧹 Nitpick | 🔵 TrivialVC serial assignment lacks
full_clean()before save.
member.save()at line 279 could raise validation errors that would be caught by the broadexcept Exceptionat line 285, but the error message would be less informative. Addingfull_clean()beforesave()would be consistent with the pattern used elsewhere in this file and produce cleaner error messages.🛠️ Suggested fix
member.serial = serial - member.save() + member.full_clean() + member.save()netbox_librenms_plugin/views/sync/interfaces.py (1)
118-136:⚠️ Potential issue | 🔴 CriticalBug:
enabledis set afterinterface.save()and never persisted.
update_interface_attributescallsinterface.save()at line 204. Control then returns here, whereenabledis mutated (lines 127-134) — but there is no subsequentsave(). Theenabledvalue is silently lost on every sync.Move the
enabledassignment before the call toupdate_interface_attributes, or add aninterface.save()after theenabledblock, or (preferred) moveenabledlogic intoupdate_interface_attributesso the singlesave()at line 204 covers everything.🐛 Preferred fix — move `enabled` into `update_interface_attributes`
Remove the post-call
enabledblock insync_interface:self.update_interface_attributes( interface, librenms_interface, netbox_type, exclude_columns, interface_name_field, ) - - if "enabled" not in exclude_columns: - interface.enabled = ( - True - if librenms_interface["ifAdminStatus"] is None - else ( - librenms_interface["ifAdminStatus"].lower() == "up" - if isinstance(librenms_interface["ifAdminStatus"], str) - else bool(librenms_interface["ifAdminStatus"]) - ) - )And add it inside
update_interface_attributes, just beforeinterface.save():self.handle_mac_address(interface, ifPhysAddress) + if "enabled" not in exclude_columns: + admin_status = librenms_interface.get("ifAdminStatus") + if admin_status is None: + interface.enabled = True + elif isinstance(admin_status, str): + interface.enabled = admin_status.lower() == "up" + else: + interface.enabled = bool(admin_status) + interface.save()This also switches from
librenms_interface["ifAdminStatus"](which raisesKeyErrorif the key is absent) to the safer.get()form.netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (1)
269-284: 🧹 Nitpick | 🔵 TrivialRecovery logic is a good fix, but the same gap exists in
pollJobStatus.The Bootstrap
getInstancerecovery here correctly bridges the stateless legacy wrapper pattern. However, everynew ModalManager(modal); manager.hide();call insidepollJobStatus(e.g., lines 371-372, 417-418, 438-439, etc.) skips this recovery step—this.instancewill always benull, so those calls always fall through to_hideManual(). If the modal was originally opened via Bootstrap, this can leave Bootstrap's internal state (scroll-lock, aria management, backdrop tracking) dirty.Consider extracting a small helper or reusing
hideModalinpollJobStatusso the recovery is consistent:Suggested helper
// Inside pollJobStatus, replace repeated pattern: - const manager = new ModalManager(modal); - manager.hide(); + hideModal(modal);netbox_librenms_plugin/import_utils.py (2)
655-666:⚠️ Potential issue | 🟡 MinorInitialize name sync fields to avoid missing-key access.
name_sync_availableandsuggested_nameare set later but not initialized in the base result. Add defaults to keep the validation shape stable for Python-side consumers.Proposed fix
"serial_confirmed": False, # True when librenms_id match and serial matches "serial_duplicate": False, # True when incoming serial is already on a different device "name_matches": False, # True when existing device name matches LibreNMS sysName + "name_sync_available": False, + "suggested_name": None, "device_type_mismatch": False, # True when existing device's type differs from LibreNMS
712-739:⚠️ Potential issue | 🟠 MajorUse
LibreNMSAPI.get_librenms_idinstead of direct custom field lookups.These queries read
custom_field_data__librenms_iddirectly, which bypasses the centralized mapping logic (multi‑server support, field access abstraction). Please switch to theLibreNMSAPI.get_librenms_idpath when resolving the mapping and use that to drive the match.As per coding guidelines: “Always call LibreNMSAPI.get_librenms_id to retrieve the device/VM LibreNMS mapping via the librenms_id custom field instead of touching the field directly.”
🤖 Fix all issues with AI agents
In `@netbox_librenms_plugin/import_utils.py`:
- Around line 2093-2120: When an existing cached object is deleted, the
recomputed is_ready uses device-only checks and incorrectly uses
device_type.matched; update _refresh_existing_device to mirror the exact
readiness logic from validate_device_for_import by branching on
validation.get("import_as_vm"): for VMs require the same "found" flags used
there (e.g., site.get("found") and device_role.get("found")), and for physical
devices require site.get("found"), device_type.get("found") and
device_role.get("found"); replace any use of device_type.get("matched") with
device_type.get("found") and use .get("found") consistently when recomputing
validation["is_ready"].
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 1110-1120: The test patches the wrong VirtualMachine target; since
validate_device_for_import does an inline/deferred import, update SERIAL_PATCHES
to patch VirtualMachine on the source import_utils module (replace
"virtualization.models.VirtualMachine" with
"netbox_librenms_plugin.import_utils.VirtualMachine" or otherwise add
"netbox_librenms_plugin.import_utils.VirtualMachine") so the deferred reference
used by validate_device_for_import/process_device_filters is properly mocked;
keep the other entries unchanged and ensure SERIAL_PATCHES includes the
import_utils.VirtualMachine string.
- Around line 1634-2099: The tests currently patch dcim.models.Device and
dcim.models.Platform but DeviceConflictActionView in
netbox_librenms_plugin.views.imports.actions holds its own imports; update all
`@patch` decorators (and any manual patches) to target
netbox_librenms_plugin.views.imports.actions.Device and
netbox_librenms_plugin.views.imports.actions.Platform so the view's references
are mocked (e.g., change patch("dcim.models.Device") to
patch("netbox_librenms_plugin.views.imports.actions.Device") and likewise for
Platform) and apply this consistently across
test_link_action_sets_librenms_id_and_name,
test_update_action_sets_hostname_serial_and_librenms_id,
test_update_serial_action_updates_serial_only, test_sync_platform_action,
test_sync_device_type_action, and any other tests in this class that mock
Device/Platform.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/views/sync/interfaces.py (1)
226-260:⚠️ Potential issue | 🟡 Minor
interface_namemay be stale orNonein the error message at Line 260.If an unexpected exception occurs during the ownership checks (Lines 234–250),
interface_namehas not yet been updated for the current iteration (it's set at Line 252). The error message would report the name from a previous iteration orNone. Move the assignment above the ownership checks whereinterfaceis already available.🛠️ Proposed fix
if object_type == "device": interface = Interface.objects.get(id=interface_id) + interface_name = interface.name if hasattr(obj, "virtual_chassis") and obj.virtual_chassis: ... else: interface = VMInterface.objects.get(id=interface_id) + interface_name = interface.name if interface.virtual_machine_id != obj.id: ... - interface_name = interface.name interface.delete()🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 226 - 260, The error handler can report a stale or None interface_name because interface_name is only set after ownership checks; move the assignment interface_name = interface.name up immediately after fetching the model instance (after Interface.objects.get(...) and VMInterface.objects.get(...)) so it is set before any ownership validation or potential exception; update the blocks inside the loop where interface is retrieved (the Interface and VMInterface branches) to assign interface_name right after the get() call and before the subsequent has/virtual_chassis or device/vm checks so the error message in the generic except uses the correct current interface name.netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (1)
269-284:⚠️ Potential issue | 🟡 Minor
hideModalcreates a throwawayModalManager—fallbackBackdropRefis never consulted.The function accepts
fallbackBackdropRef(and callers likeinitializeHTMXHandlerspass it), but the newModalManagerinstance hasbackdropElement = nulland_hideManual()never checksfallbackBackdropRef. If Bootstrap is unavailable and the backdrop was created by a differentModalManager(viashowModal),_hideManualfalls back todocument.querySelector('.modal-backdrop')— which works only if there's a single backdrop in the DOM. With two modals open simultaneously (e.g., filter modal + HTMX modal), this could remove the wrong backdrop.Consider either reusing the original
ModalManagerinstance or actually wiringfallbackBackdropRefinto the hide path:♻️ Suggested approach
function hideModal(modalElement, fallbackBackdropRef) { if (!modalElement) { return; } const manager = new ModalManager(modalElement); + // Restore backdrop reference from showModal so _hideManual removes the correct one + if (fallbackBackdropRef && fallbackBackdropRef.element) { + manager.backdropElement = fallbackBackdropRef.element; + } + // Try to recover an existing Bootstrap instance before falling back to manual if (typeof bootstrap !== 'undefined' && bootstrap.Modal) { manager.instance = bootstrap.Modal.getInstance(modalElement); } else if (typeof window.bootstrap !== 'undefined' && window.bootstrap.Modal) { manager.instance = window.bootstrap.Modal.getInstance(modalElement); } manager.hide(); + + // Clear the backdrop ref after successful hide + if (fallbackBackdropRef) { + fallbackBackdropRef.element = null; + } }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js` around lines 269 - 284, hideModal creates a new ModalManager and never uses the passed fallbackBackdropRef, so when bootstrap is unavailable the manual hide path (_hideManual) can remove the wrong backdrop; update hideModal (or its call sites like initializeHTMXHandlers) to either retrieve and reuse the original ModalManager instance created by showModal or pass the original manager.backdropElement into the new manager before calling manager.hide(); specifically ensure fallbackBackdropRef (the DOM reference to the correct backdrop) is wired into ModalManager.backdropElement (or provided to _hideManual) so the manual fallback uses that element instead of document.querySelector('.modal-backdrop').
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@netbox_librenms_plugin/import_utils.py`:
- Around line 728-755: The code sets name_sync_available and suggested_name for
Device matches by librenms_id but not for VMs (existing_vm handling only sets
name_matches); add a brief clarifying comment in import_utils.py near the
existing_vm and existing_device branches (referencing existing_vm,
existing_device, result["name_matches"], result["name_sync_available"],
result["suggested_name"]) stating this asymmetry is intentional because
UpdateDeviceNameView only supports Device objects and VM name-sync is not
implemented, so maintainers should not assume parity when extending behavior for
VMs without adding VM name-sync support.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 269-284: hideModal creates a new ModalManager and never uses the
passed fallbackBackdropRef, so when bootstrap is unavailable the manual hide
path (_hideManual) can remove the wrong backdrop; update hideModal (or its call
sites like initializeHTMXHandlers) to either retrieve and reuse the original
ModalManager instance created by showModal or pass the original
manager.backdropElement into the new manager before calling manager.hide();
specifically ensure fallbackBackdropRef (the DOM reference to the correct
backdrop) is wired into ModalManager.backdropElement (or provided to
_hideManual) so the manual fallback uses that element instead of
document.querySelector('.modal-backdrop').
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 104-107: selected_device_id from POST can be non-numeric and
Device.objects.get(id=selected_device_id) may raise ValueError/TypeError (not
just Device.DoesNotExist); update the exception handling around
Device.objects.get (the block that assigns target_device from
Device.objects.get(id=selected_device_id) and falls back to obj) to either
validate/convert selected_device_id to an int before calling Device.objects.get
or expand the except clause to also catch ValueError and TypeError in addition
to Device.DoesNotExist so invalid IDs are handled safely and target_device is
set to obj on error.
- Around line 226-260: The error handler can report a stale or None
interface_name because interface_name is only set after ownership checks; move
the assignment interface_name = interface.name up immediately after fetching the
model instance (after Interface.objects.get(...) and
VMInterface.objects.get(...)) so it is set before any ownership validation or
potential exception; update the blocks inside the loop where interface is
retrieved (the Interface and VMInterface branches) to assign interface_name
right after the get() call and before the subsequent has/virtual_chassis or
device/vm checks so the error message in the generic except uses the correct
current interface name.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
netbox_librenms_plugin/import_utils.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuse thelibrenms_api.pyLibreNMS client instead of making newrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching
Always callLibreNMSAPI.get_librenms_idto retrieve the device/VM LibreNMS mapping via thelibrenms_idcustom field instead of touching the field directly
Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functionsfind_matching_site,match_librenms_hardware_to_device_type, andfind_matching_platformfromutils.py
Files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils.py
🧠 Learnings (11)
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modal buttons should target the `htmx-modal-content` element. JavaScript in `librenms_import.html` toggles the wrapper; do not reintroduce `data-bs-toggle` or duplicate modal IDs.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Avoid `outerHTML` swaps in HTMX; use OOB or targeted `innerHTML` swaps to keep table layout intact.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Keep `<select class="device-role-select">` markup stable to preserve JavaScript hook-up with TomSelect decorators.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Update modal messages based on RQ status values: 'Job queued...', 'Processing...', 'Job completed!' with explicit handling for all RQ status values to avoid infinite polling
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/netbox_librenms_plugin/*sync*.html : Sync pages should extend `librenms_sync_base.html`.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/device_fields.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
Applied to files:
netbox_librenms_plugin/views/sync/device_fields.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to {navigation.py,urls.py,api/**} : Respect NetBox plugin APIs in `navigation.py`, `urls.py`, and `api/` directories
Applied to files:
netbox_librenms_plugin/views/sync/device_fields.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Follow sync pipeline flow: fetch LibreNMS data via `librenms_api.py`, cache it using `CacheMixin`, build comparison tables in `tables/`, and render HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/import_utils.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_import_utils.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`
Applied to files:
netbox_librenms_plugin/import_utils.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to **/*.py : Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functions `find_matching_site`, `match_librenms_hardware_to_device_type`, and `find_matching_platform` from `utils.py`
Applied to files:
netbox_librenms_plugin/import_utils.py
🧬 Code graph analysis (2)
netbox_librenms_plugin/views/sync/interfaces.py (3)
netbox_librenms_plugin/views/sync/cables.py (1)
get_selected_interfaces(16-28)netbox_librenms_plugin/utils.py (1)
convert_speed_to_kbps(12-24)netbox_librenms_plugin/models.py (1)
InterfaceTypeMapping(51-76)
netbox_librenms_plugin/views/sync/device_fields.py (3)
netbox_librenms_plugin/utils.py (1)
match_librenms_hardware_to_device_type(208-256)netbox_librenms_plugin/views/mixins.py (2)
LibreNMSAPIMixin(4-90)librenms_api(25-39)netbox_librenms_plugin/librenms_api.py (2)
get_librenms_id(173-229)get_device_info(310-333)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
🔇 Additional comments (15)
netbox_librenms_plugin/views/sync/interfaces.py (2)
129-140: Good fix —.get()usage addresses the previousKeyErrorconcern.The switch from direct dict access to
.get()on Lines 131–132 preventsKeyErrorwhenifSpeedorifTypeis absent from the LibreNMS port data. The fallback behavior throughconvert_speed_to_kbps(None) → Noneand the queryset filtering onNonetype is handled gracefully downstream.[approve_code_changes, duplicate_comment]
193-199: Enabled-flag logic is correct and handles edge cases well.The three-way handling (None → default-on, string → case-insensitive check, other → bool coercion) is a solid approach for the varying
ifAdminStatusshapes that LibreNMS can return.Minor readability suggestion: the nested ternary could be extracted to a small helper or use an early-assign pattern for clarity, but this is not blocking.
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (2)
304-304: Good improvement:filterModalis now cached once at the top ofpollJobStatus.This eliminates ~9 redundant
getElementByIdcalls across the cancel handler and poll callback, exactly as previously suggested.
965-967: LGTM — auto-close of results modal useshideModalcorrectly.Single modal scenario, no backdrop reference needed.
netbox_librenms_plugin/import_utils.py (6)
661-667: Well-structured validation state fields.The new per-field flags (
serial_confirmed,serial_duplicate,name_matches,name_sync_available,device_type_mismatch) provide clear, granular state for the conflict resolution UI. Defaults are sensible.
757-779: Serial drift logic correctly separates conflict from update — past review addressed.When
serial_conflictexists,serial_actionis set to"conflict"(not"update_serial"), preventing duplicate serial assignment. Good fix.One minor gap: if the existing device has no serial (
existing_device.serialis empty) but LibreNMS reports one, neither branch triggers — no action is surfaced to populate the serial. This may be acceptable if serial population is handled elsewhere in the sync workflow.
864-883: IP lookup now usesaddress__net_host— past review fix applied.The
startswithfalse-positive issue is resolved.
1026-1058: Readiness logic is clean and correctly differentiates VM vs Device requirements.The existing-device path correctly short-circuits
is_ready = Falseand populates the role from the matched device for modal display. The device-type mismatch detection (line 1036–1044) is a useful addition for surfacing hardware model changes.
2293-2296: Good: stale device data is refreshed from DB before reuse.Placing
_refresh_existing_deviceimmediately after loading from cache ensures NetBox-side changes (role, name, deletion) are reflected in the import table without a full re-validation cycle.
781-838: Hostname cross-check logic handles ambiguity well.The three-way check (both VM+Device, VM only, Device only) correctly avoids false matches when the same hostname exists as both types. The warning message guides users to resolve via
librenms_idcustom field. Serial conflict handling in the hostname branch now correctly differentiates"conflict"vs"update_serial".netbox_librenms_plugin/views/sync/device_fields.py (5)
12-49:UpdateDeviceNameViewis well-implemented with proper validation and rollback.Uses
get_librenms_id(compliant with coding guidelines), validates sysName availability, runsfull_clean()beforesave(), and reverts toold_nameon failure — matching the pattern suggested in the prior review.
78-85: Serial update now validates before save and reverts on failure.Consistent
full_clean()/save()pattern with proper rollback toold_serial.
134-141: Device type update now validates before save and reverts on failure.Consistent error handling pattern.
238-259: Platform creation guards against slug collisions and assignment failures.The two-stage error handling (creation then assignment) correctly isolates failures. On assignment failure, the device reverts to
old_platform(line 256) — this is the fix previously requested.One subtlety: if platform creation succeeds (line 239–242) but device assignment fails (line 255), the newly created platform remains orphaned in the database. This is acceptable since orphaned platforms are harmless and the user is informed of the failure, but worth noting if cleanup is ever desired.
301-308: Per-member serial validation with error aggregation is a solid improvement.Each VC member gets
full_clean()/save()independently, so one failure doesn't block others. Errors are collected and displayed per-member (lines 325–327).
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 (2)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (1)
760-783: 🧹 Nitpick | 🔵 TrivialTwo different hiding strategies for the same modal.
initializeFilterFormuses the long-livedfilterModalManager.hide()(lines 776, 780) for synchronous/error paths, but delegates topollJobStatusfor background jobs, which uses the standalonehideModal(filterModal)with an ephemeralModalManager. Both paths target the samefilter-processing-modalelement.This works because
hideModalrecovers the Bootstrap instance viagetInstance, but the split makes it easy to introduce subtle bugs if either path changes. Consider passingfilterModalManager(or a hide callback) intopollJobStatusto unify the modal lifecycle.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js` around lines 760 - 783, The modal is hidden two different ways which risks lifecycle bugs; update initializeFilterForm to pass the existing filterModalManager (or a hide callback) into pollJobStatus instead of relying on pollJobStatus’s ephemeral hideModal/ModalManager logic, and then change pollJobStatus to call filterModalManager.hide() (or the provided callback) when it needs to close the "filter-processing-modal" instead of using hideModal/getInstance; reference initializeFilterForm, pollJobStatus, hideModal, ModalManager, filterModalManager and the "filter-processing-modal" element when making this change.netbox_librenms_plugin/views/sync/interfaces.py (1)
142-152:⚠️ Potential issue | 🟠 Major
MACAddress.objects.createwill create duplicate MAC address records across the database.Line 145 only checks MAC addresses already associated with this interface. If the same physical address is synced for a different interface (e.g., NIC migrated between devices),
create()at line 149 will silently create a duplicateMACAddressrecord. Usingget_or_createon the globalMACAddresstable avoids this duplication and is the standard Django pattern for this scenario.🛠️ Proposed fix
def handle_mac_address(self, interface, ifPhysAddress): """Assign or create the MAC address for the given interface.""" if ifPhysAddress: - existing_mac = interface.mac_addresses.filter(mac_address=ifPhysAddress).first() - if existing_mac: - mac_obj = existing_mac - else: - mac_obj = MACAddress.objects.create(mac_address=ifPhysAddress) + mac_obj, _ = MACAddress.objects.get_or_create(mac_address=ifPhysAddress) interface.mac_addresses.add(mac_obj) interface.primary_mac_address = mac_obj🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 142 - 152, The handle_mac_address method currently creates a new MACAddress only after checking addresses related to the given interface, which can create duplicate MACAddress rows; change the creation to use MACAddress.objects.get_or_create(mac_address=ifPhysAddress) (inside handle_mac_address) so you deduplicate globally, then use the returned mac object (and ignore the created flag) when calling interface.mac_addresses.add(...) and assigning interface.primary_mac_address.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 271-286: hideModal creates a new throwaway ModalManager so it
cannot see the backdrop tracked by the ModalManager created in showModal;
persist the manager per modal element (e.g., introduce a WeakMap keyed by the
modal DOM element) and use that stored ModalManager instance in both showModal
and hideModal instead of instantiating a new ModalManager each call so that
ModalManager.hide() and its _hideManual fallback operate on the same tracked
backdrop element.
- Around line 760-783: The modal is hidden two different ways which risks
lifecycle bugs; update initializeFilterForm to pass the existing
filterModalManager (or a hide callback) into pollJobStatus instead of relying on
pollJobStatus’s ephemeral hideModal/ModalManager logic, and then change
pollJobStatus to call filterModalManager.hide() (or the provided callback) when
it needs to close the "filter-processing-modal" instead of using
hideModal/getInstance; reference initializeFilterForm, pollJobStatus, hideModal,
ModalManager, filterModalManager and the "filter-processing-modal" element when
making this change.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 142-152: The handle_mac_address method currently creates a new
MACAddress only after checking addresses related to the given interface, which
can create duplicate MACAddress rows; change the creation to use
MACAddress.objects.get_or_create(mac_address=ifPhysAddress) (inside
handle_mac_address) so you deduplicate globally, then use the returned mac
object (and ignore the created flag) when calling
interface.mac_addresses.add(...) and assigning interface.primary_mac_address.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
netbox_librenms_plugin/import_utils.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/views/sync/interfaces.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuse thelibrenms_api.pyLibreNMS client instead of making newrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching
Always callLibreNMSAPI.get_librenms_idto retrieve the device/VM LibreNMS mapping via thelibrenms_idcustom field instead of touching the field directly
Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functionsfind_matching_site,match_librenms_hardware_to_device_type, andfind_matching_platformfromutils.py
Files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/import_utils.py
🧠 Learnings (10)
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modal buttons should target the `htmx-modal-content` element. JavaScript in `librenms_import.html` toggles the wrapper; do not reintroduce `data-bs-toggle` or duplicate modal IDs.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Avoid `outerHTML` swaps in HTMX; use OOB or targeted `innerHTML` swaps to keep table layout intact.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Keep `<select class="device-role-select">` markup stable to preserve JavaScript hook-up with TomSelect decorators.
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Update modal messages based on RQ status values: 'Job queued...', 'Processing...', 'Job completed!' with explicit handling for all RQ status values to avoid infinite polling
Applied to files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/netbox_librenms_plugin/*sync*.html : Sync pages should extend `librenms_sync_base.html`.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/import_utils.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Follow sync pipeline flow: fetch LibreNMS data via `librenms_api.py`, cache it using `CacheMixin`, build comparison tables in `tables/`, and render HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_import_utils.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`
Applied to files:
netbox_librenms_plugin/import_utils.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to **/*.py : Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functions `find_matching_site`, `match_librenms_hardware_to_device_type`, and `find_matching_platform` from `utils.py`
Applied to files:
netbox_librenms_plugin/import_utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-netbox (3.12)
🔇 Additional comments (10)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (3)
371-373: Consistent use of centralizedhideModalacross all poll/cancel code paths.The guard-then-hide pattern (
if (filterModal) { hideModal(filterModal); }) is applied uniformly. No issues.Also applies to: 414-416, 544-546
965-971: LGTM —hideModalfor results modal auto-close.Correctly hides the import results modal after a successful import with zero failures.
267-270: WONTFIX comment is clear and well-placed.Good documentation of the architectural constraint (single modal at a time in Tabler) that makes
querySelectorsafe here. Based on learnings: "Modals use Tabler (Bootstrap-like) but withoutbootstrap.Modalhelpers."netbox_librenms_plugin/import_utils.py (4)
728-782: LGTM — serial conflict and name sync logic looks correct.The serial drift handling properly differentiates between conflict (duplicate serial on another device) and update (serial changed on same device). The VM name_sync comment clarifies the intentional asymmetry.
867-886: LGTM — IP matching usesaddress__net_hostfor exact host match.The previous false-positive-prone
address__startswithhas been replaced withaddress__net_host, which correctly matches only the host portion of CIDR addresses.
1029-1061: LGTM — readiness logic properly branches on import type.VMs require only cluster; devices require site, device_type, and device_role. The
can_importcheck correctly uses the issues list.
2296-2298: LGTM — refresh of cached existing devices before further processing.This ensures NetBox-side changes (role, name, deletion) are reflected before applying filters.
netbox_librenms_plugin/views/sync/interfaces.py (3)
193-199: Enabled logic consolidation looks correct.The enabled-status handling now lives solely in
update_interface_attributes, eliminating the previously flagged double-save. The three-way handling (None → defaultTrue, string → case-insensitive"up"check, other →bool()) is sound. The nested conditional expression is dense but acceptable given the straightforward branching.
226-261: Good defensive initialization ofinterface_name.Initializing
interface_name = Nonebefore the loop ensures the generic error handler at line 261 won't hit anUnboundLocalErroron the first iteration if an unexpected exception fires before the name is assigned.
19-50: Docstrings and overall flow look clean.The added docstrings across all methods provide useful context. The sync pipeline correctly follows the pattern: fetch cached data → gate on selection → sync within a transaction → redirect with messaging.
…custom field Devcontainer & CI: - Add proxy/CA bundle support with ALLOW_GIT_SSL_DISABLE opt-in - Add Codespaces configuration loader - Remove unnecessary proxy env vars from postgres/redis services - Extract detect_plugin_workspace() helper, idempotent .bashrc guard - Consolidate aliases into load-aliases.sh as single source of truth - Fix CI test workflow to run from correct NetBox directory - Add media/configuration.testing.py for CI - Update lint workflow: actions v4/v5, Python 3.12, fail on lint errors - Exclude tests from package distribution - Fix MD031 markdown lint in README - Add security note about embedding proxy credentials in URLs Plugin: - Auto-create librenms_id custom field via post_migrate signal - Log exceptions instead of silently swallowing them in custom field creation - Add inline comments on _executed flag lifecycle assumptions - Raise KeyError for non-default missing server keys in LibreNMSAPI Tests: - Add setup_method for consistent _executed flag reset - Assert exception logging in test_exception_does_not_propagate - Fix fragile getLogger assertion in test_no_log_when_field_already_exists
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/lint-format.yaml (1)
22-22: 🧹 Nitpick | 🔵 TrivialPin the
ruffversion for reproducible CI.An unpinned
ruffmeans a new release with rule changes can break CI without any code change. Pin to the version the team currently uses (e.g.,ruff==0.9.x).Proposed fix
- pip install ruff + pip install ruff==0.9.7Replace
0.9.7with whatever version you're currently using locally / inpyproject.toml.latest ruff version 2026🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint-format.yaml at line 22, The CI step currently uses the unpinned command 'pip install ruff'; replace that with a pinned version matching the team's ruff version (for example change 'pip install ruff' to 'pip install ruff==<VERSION>' where <VERSION> is taken from pyproject.toml or your local setup, e.g., 0.9.x) so CI is reproducible and won’t break on upstream ruff releases.netbox_librenms_plugin/tests/test_librenms_api.py (1)
620-641: 🧹 Nitpick | 🔵 TrivialTest relies on MagicMock's no-op
raise_for_status— consider making the mock explicit.Setting
mock_post.return_value.status_code = 500has no effect becauseMagicMock.raise_for_status()is a no-op by default. The test passes becauseresponse.json()returns{"status": "error", ...}, not because HTTP 500 handling is exercised. Ifraise_for_statuswere made realistic (viaresponse.raise_for_status.side_effect = requests.exceptions.HTTPError(...)), the test would follow a different code path.This is a pre-existing pattern, but worth noting for future test hygiene.
🧪 Suggested: make raise_for_status realistic for 500
`@patch`("netbox_librenms_plugin.librenms_api.requests.post") def test_add_device_duplicate_error(self, mock_post, mock_librenms_config): """Verify duplicate device handling.""" - mock_post.return_value.status_code = 500 - mock_post.return_value.json.return_value = { - "status": "error", - "message": "Device already exists", - } + mock_response = MagicMock() + mock_response.status_code = 500 + mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError( + "500 Server Error", response=mock_response + ) + mock_post.return_value = mock_responseThis would exercise the
except RequestExceptionpath, and you'd assert onstr(e)content instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_librenms_api.py` around lines 620 - 641, The test test_add_device_duplicate_error currently relies on MagicMock's no-op raise_for_status, so set mock_post.return_value.raise_for_status.side_effect = requests.exceptions.HTTPError("500 Server Error") (or set it to a no-op explicitly and keep status_code 200) to make the HTTP 500 behavior explicit; update the assertions for LibreNMSAPI.add_device to match the resulting path (either assert the RequestException-based error message when raise_for_status raises, or keep the JSON error path by returning a 200 and a body with {"status":"error",...}). Ensure you reference mock_post in the test and LibreNMSAPI.add_device when making the change.netbox_librenms_plugin/views/sync/interfaces.py (1)
264-301:⚠️ Potential issue | 🟡 Minor
interface_namemay carry a stale value from a previous loop iteration in the genericexcept.If
Interface.objects.get(id=interface_id)on line 273 raises an unexpected exception (before line 274 assignsinterface_name), the genericexcepton line 300 will useinterface_namefrom the previous iteration (orNoneon the first). While extremely unlikely for a PK lookup, resettinginterface_nameat the top of each iteration would be more defensive.🛡️ Proposed fix
for interface_id in interface_ids: try: + interface_name = f"ID {interface_id}" if object_type == "device": interface = Interface.objects.get(id=interface_id) interface_name = interface.name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 264 - 301, The loop over interface_ids can leave interface_name stale if an unexpected exception occurs before it is assigned; inside the for interface_id in interface_ids loop, reset interface_name (e.g., set interface_name = None) at the top of each iteration before calling Interface.objects.get or VMInterface.objects.get so the generic except Exception as exc will not report a stale name when handling errors.netbox_librenms_plugin/forms.py (1)
295-328: 🧹 Nitpick | 🔵 TrivialDuplicated
_get_poller_group_choicesbetween SNMP forms.
AddToLIbreSNMPV1V2._get_poller_group_choices(lines 301–328) andAddToLIbreSNMPV3._get_poller_group_choices(lines 429–456) are identical. Extract to a module-level helper (like the existing_get_librenms_server_choices) to eliminate duplication.♻️ Proposed refactor
+def _get_poller_group_choices(): + """Get poller group choices from LibreNMS API.""" + from .librenms_api import LibreNMSAPI + + choices = [("0", "Default (0)")] + try: + api = LibreNMSAPI() + success, poller_groups = api.get_poller_groups() + if success and poller_groups: + for group in poller_groups: + group_id = str(group.get("id", "")) + group_name = group.get("group_name", "") + group_descr = group.get("descr", "") + if group_id: + if group_descr and group_descr != group_name: + label = f"{group_name} - {group_descr} ({group_id})" + else: + label = f"{group_name} ({group_id})" + choices.append((group_id, label)) + except Exception: + pass + return choicesThen in both form
__init__methods:- self.fields["poller_group"].choices = self._get_poller_group_choices() + self.fields["poller_group"].choices = _get_poller_group_choices()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/forms.py` around lines 295 - 328, The two identical methods AddToLIbreSNMPV1V2._get_poller_group_choices and AddToLIbreSNMPV3._get_poller_group_choices should be extracted to a shared module-level helper (similar to the existing _get_librenms_server_choices); create a new function (e.g., _get_librenms_poller_group_choices) that contains the current API call and choice-building logic, replace both class methods to call that helper from their __init__ (set self.fields["poller_group"].choices = _get_librenms_poller_group_choices()), and remove the duplicated method definitions from both AddToLIbreSNMPV1V2 and AddToLIbreSNMPV3 to eliminate duplication.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/README.md:
- Around line 160-235: Fix the minor grammar in the MITM proxy section under the
"### 🌐 Proxy Configuration (MITM Proxies)" heading: change the sentence "If
you're behind a corporate proxy or MITM proxy (like Zscaler, BlueCoat, etc.),
you need to configure proxy at two levels: the Docker client (for building) and
the container runtime (for package installation inside the container)." to
include "the" before "proxy" so it reads "you need to configure the proxy at two
levels"; update that exact sentence in the README.md fragment to preserve tone
and punctuation.
In @.devcontainer/scripts/load-aliases.sh:
- Around line 64-65: Replace the long single-line alias `dev-help` with a shell
function named `dev-help` that prints the same multi-line help text;
specifically, remove the alias definition and add a function `dev-help() { ...
}` that emits the formatted help (use a heredoc or multiple echo lines) so the
help is readable and maintainable, and ensure the function is defined in the
same script so it’s available when the script is sourced.
In @.github/copilot-instructions.md:
- Around line 29-37: Add a blank line immediately before the "## Permission
System" heading and another blank line immediately before the "## When in Doubt"
heading in .github/copilot-instructions.md so each top-level section is
separated by an empty line; specifically insert an empty line after the
preceding paragraph ending before "## Permission System" and after line 37 so
"## When in Doubt" is preceded by a blank line.
In @.github/instructions/background-jobs.instructions.md:
- Around line 30-35: Add a blank line after the "Superuser Requirement for
Background Jobs" heading block to satisfy markdownlint MD022; update the section
following the paragraph that references BaseRQViewSet, IsSuperuser, and the
should_use_background_job() helpers in list.py and actions.py so there is an
empty line separating the heading from the next section/content.
In @.github/instructions/frontend.instructions.md:
- Around line 19-22: Add a blank line immediately after the heading "##
JavaScript Fetch Patterns" to satisfy MD022; open the
.github/instructions/frontend.instructions.md content around the "## JavaScript
Fetch Patterns" heading and insert one empty line before the first bullet so the
heading is separated from the list.
In @.github/workflows/lint-format.yaml:
- Around line 3-5: The GitHub Actions workflow currently triggers on every push
and every pull_request because the top-level keys "push" and "pull_request" have
no branch filters; to limit CI noise, update the workflow trigger block (the
"on:" section) to add branch filters such as specifying push: branches: [ "main"
] and pull_request: branches: [ "main" ] (or your repo's default branch) so the
workflow runs only for pushes to main and PRs targeting main; alternatively, if
the intention is to run on all branches, leave as-is and document that decision
in the workflow header.
- Line 22: The CI step currently uses the unpinned command 'pip install ruff';
replace that with a pinned version matching the team's ruff version (for example
change 'pip install ruff' to 'pip install ruff==<VERSION>' where <VERSION> is
taken from pyproject.toml or your local setup, e.g., 0.9.x) so CI is
reproducible and won’t break on upstream ruff releases.
In @.github/workflows/test.yaml:
- Around line 64-71: In the "Set up configuration" step update the ln command so
the command substitution is quoted to prevent word-splitting when the runner
path contains spaces; change the call using $(pwd) to use a quoted form like
"$(pwd)" (or "$PWD") when constructing the symlink target so the ln invocation
(the line creating configuration.py) is safe with paths containing spaces.
- Around line 51-56: The workflow title/behavior is misleading: the checkout
step currently pins NetBox to ref "main" (actions/checkout@v4 with repository
"netbox-community/netbox" and ref: main) but the job matrix only varies Python
versions; update the workflow to either add a NetBox version matrix axis (e.g.,
add a matrix entry like netbox-ref: [main, v4.2.5, v4.1.10] and replace the
hardcoded ref: main with ref: ${{ matrix.netbox-ref }}) or change the workflow
name to remove the “all supported NetBox versions” claim so it matches the
current behavior.
In `@docs/usage_tips/permissions.md`:
- Around line 60-62: Update the permission list so all object permissions use
the fully-qualified prefix; replace the short-form entries
`change_interfacetypemapping` and `delete_interfacetypemapping` with
`netbox_librenms_plugin.change_interfacetypemapping` and
`netbox_librenms_plugin.delete_interfacetypemapping`, keeping the existing
`netbox_librenms_plugin.add_interfacetypemapping` entry unchanged.
- Around line 83-84: The markdown has adjacent headings "## Further Details" and
"### Tier 1: Plugin Permissions" with no blank line (MD022); open the document
and insert a single blank line between the "## Further Details" heading and the
"### Tier 1: Plugin Permissions" subheading so there is an empty line separating
the headings to satisfy the lint rule.
In `@netbox_librenms_plugin/api/views.py`:
- Around line 21-33: The permission check in
LibreNMSPluginPermission.has_permission currently treats only "GET" as read-only
causing HEAD/OPTIONS to require PERM_CHANGE_PLUGIN; modify has_permission to
treat DRF's SAFE_METHODS as read-only by checking if request.method is in
SAFE_METHODS and return request.user.has_perm(PERM_VIEW_PLUGIN), otherwise
return request.user.has_perm(PERM_CHANGE_PLUGIN); use the SAFE_METHODS symbol
from rest_framework.permissions to implement this change.
In `@netbox_librenms_plugin/forms.py`:
- Around line 295-328: The two identical methods
AddToLIbreSNMPV1V2._get_poller_group_choices and
AddToLIbreSNMPV3._get_poller_group_choices should be extracted to a shared
module-level helper (similar to the existing _get_librenms_server_choices);
create a new function (e.g., _get_librenms_poller_group_choices) that contains
the current API call and choice-building logic, replace both class methods to
call that helper from their __init__ (set self.fields["poller_group"].choices =
_get_librenms_poller_group_choices()), and remove the duplicated method
definitions from both AddToLIbreSNMPV1V2 and AddToLIbreSNMPV3 to eliminate
duplication.
In `@netbox_librenms_plugin/tables/interfaces.py`:
- Around line 55-57: The lambda assigned to "data-enabled" can raise
AttributeError when record["ifAdminStatus"] is non-string; update it to guard
like _parse_enabled_status does (e.g., check isinstance(value, str) before
calling .lower()) or simply call
_parse_enabled_status(record.get("ifAdminStatus")) so non-string values return a
safe empty string instead of crashing; target the "data-enabled" lambda in
tables/interfaces.py and reuse or mirror the logic from _parse_enabled_status to
fix this.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html`:
- Around line 436-454: The savePref function currently does a fire-and-forget
fetch which can fail with unhandled rejections or 403s if the CSRF token is
missing; update savePref to first retrieve and validate the CSRF token (from the
csrfmiddlewaretoken input or cookie) and if missing bail out or report an error,
then use fetch with credentials: 'same-origin', await the response, check
response.ok and handle non-2xx status (log to console or show a non-blocking UI
error), and wrap the await fetch call in try/catch to catch network exceptions;
ensure the existing event listeners on elements with IDs 'use-sysname-toggle'
and 'strip-domain-toggle' still call the updated savePref.
In `@netbox_librenms_plugin/tests/test_init.py`:
- Around line 43-44: The test currently patches logging.getLogger broadly which
can interfere with other logger creation; instead patch the specific logger used
by the module (or the cached logger attribute) when calling
_ensure_librenms_id_custom_field. Replace patch("logging.getLogger") with a
patch targeting the module logger (e.g. patch("netbox_librenms_plugin.logger")
or patch.object(netbox_librenms_plugin, "logger") after importing the module),
use the patched logger's methods (like mock_logger.info/exception) for
assertions, and update the assert to check the module logger was called with the
expected messages rather than asserting logging.getLogger was called.
In `@netbox_librenms_plugin/tests/test_librenms_api.py`:
- Around line 620-641: The test test_add_device_duplicate_error currently relies
on MagicMock's no-op raise_for_status, so set
mock_post.return_value.raise_for_status.side_effect =
requests.exceptions.HTTPError("500 Server Error") (or set it to a no-op
explicitly and keep status_code 200) to make the HTTP 500 behavior explicit;
update the assertions for LibreNMSAPI.add_device to match the resulting path
(either assert the RequestException-based error message when raise_for_status
raises, or keep the JSON error path by returning a 200 and a body with
{"status":"error",...}). Ensure you reference mock_post in the test and
LibreNMSAPI.add_device when making the change.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 819-822: DeviceConflictActionView lacks the
LibreNMSPermissionMixin and does not call require_write_permission(), so POST
requests can mutate Device records without authorization; update the class
definition to include LibreNMSPermissionMixin (alongside LibreNMSAPIMixin and
DeviceImportHelperMixin) and add a require_write_permission() call at the start
of its post(self, request, device_id) method before performing any saves/updates
(the symbols to change are DeviceConflictActionView and its post method; reuse
the same pattern used in
BulkImportConfirmView/BulkImportDevicesView/DeviceRoleUpdateView).
In `@netbox_librenms_plugin/views/imports/list.py`:
- Around line 310-318: The fallback for use_sysname currently defaults to False
when settings is None, which diverges from the model default
(LibreNMSSettings.use_sysname_default=True); update the fallback logic in the
block that reads user prefs (the use_sysname assignment using _get_user_pref and
getattr(settings, "use_sysname_default", ...)) so that when settings is None it
falls back to True instead of False; leave strip_domain fallback as-is (or
verify it matches LibreNMSSettings.strip_domain_default) and ensure you
reference the same symbols (use_sysname, _get_user_pref, settings,
LibreNMSSettings.use_sysname_default) when making the change.
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 55-59: The current check_existing_cable method
(check_existing_cable) only filters by terminations__termination_id and
therefore can match other termination types; restrict the search to Interface
terminations by adding the termination type check to the query (e.g. include
terminations__termination_type__model='interface' in each Q clause or otherwise
constrain terminations__termination_type to the Interface ContentType) so that
Cable.objects.filter(...) only considers Interface terminations when checking
for existing cables between local_interface and remote_interface.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 19-25: The method post has an extra blank line causing ruff format
to fail; inside the post method (function post) collapse the double blank line
between the require_all_permissions check (require_all_permissions) and the
Device retrieval (get_object_or_404(Device, pk=pk)) into a single blank line so
the body has only one blank line there and matches project formatting rules.
In `@netbox_librenms_plugin/views/sync/devices.py`:
- Around line 24-29: get_object currently returns
Device.objects.get(pk=object_id) and, on Device.DoesNotExist, calls
VirtualMachine.objects.get(pk=object_id) which can raise an uncaught
VirtualMachine.DoesNotExist and produce a 500; update get_object to handle the
fallback safely by using get_object_or_404 for the VirtualMachine lookup or by
catching both Device.DoesNotExist and VirtualMachine.DoesNotExist and raising
Http404; locate the get_object function and replace the fallback call to
VirtualMachine.objects.get with a call to get_object_or_404(VirtualMachine,
pk=object_id) or add a second except block for VirtualMachine.DoesNotExist to
raise Http404.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 211-217: The nested ternary setting interface.enabled is hard to
read; refactor the block that checks exclude_columns and reads admin_status
(from librenms_interface.get("ifAdminStatus")) to use a clear if/elif/else:
first handle admin_status is None -> True, then if isinstance(admin_status, str)
compare lower() == "up", else cast to bool(admin_status), and assign the result
to interface.enabled. This keeps the same behavior but replaces the dense nested
conditional with a straightforward sequence using the existing symbols
admin_status, librenms_interface.get, exclude_columns, and interface.enabled.
- Around line 264-301: The loop over interface_ids can leave interface_name
stale if an unexpected exception occurs before it is assigned; inside the for
interface_id in interface_ids loop, reset interface_name (e.g., set
interface_name = None) at the top of each iteration before calling
Interface.objects.get or VMInterface.objects.get so the generic except Exception
as exc will not report a stale name when handling errors.
| # Help | ||
| alias dev-help='echo "🎯 NetBox LibreNMS Plugin Development Commands:"; echo ""; echo "📊 NetBox Server Management:"; echo " netbox-run-bg : Start NetBox in background"; echo " netbox-run : Start NetBox in foreground (for debugging)"; echo " netbox-stop : Stop NetBox and RQ worker"; echo " netbox-restart : Restart NetBox and RQ worker"; echo " netbox-reload : Reinstall plugin and restart NetBox"; echo " netbox-status : Check if NetBox and RQ worker are running"; echo " netbox-logs : View NetBox server logs"; echo ""; echo "⚙️ Background Jobs (RQ Worker):"; echo " rq-status : Check if RQ worker is running"; echo " rq-logs : View RQ worker logs"; echo " rq-stats : Show RQ queue statistics"; echo " rq-jobs : List jobs in default queue"; echo " rq-failed : List failed jobs"; echo " rq-recent : Show recent NetBox jobs"; echo ""; echo "🛠️ Development Tools:"; echo " netbox-shell : Open NetBox Django shell"; echo " netbox-test : Run plugin tests"; echo " netbox-manage : Run Django management commands"; echo " plugin-install : Reinstall plugin in development mode"; echo ""; echo "🧹 Code Quality:"; echo " ruff-check : Check code with Ruff"; echo " ruff-format : Format code with Ruff"; echo " ruff-fix : Auto-fix code issues with Ruff"; echo ""; echo "🔎 Diagnostics:"; echo " diagnose : Run startup diagnostics"; echo " dev-help : Show this help message"; echo ""; echo "📖 NetBox available at: http://localhost:8000 (admin/admin)"; echo ""' |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a shell function instead of a single-line alias for readability.
The dev-help alias works, but the single-line echo chain is hard to maintain. A function would let you format the help text across multiple lines while keeping the same behavior.
♻️ Optional: refactor to a function
# Help
-alias dev-help='echo "🎯 NetBox LibreNMS Plugin Development Commands:"; echo ""; echo "📊 NetBox Server Management:"; echo " netbox-run-bg : Start NetBox in background"; echo " netbox-run : Start NetBox in foreground (for debugging)"; echo " netbox-stop : Stop NetBox and RQ worker"; echo " netbox-restart : Restart NetBox and RQ worker"; echo " netbox-reload : Reinstall plugin and restart NetBox"; echo " netbox-status : Check if NetBox and RQ worker are running"; echo " netbox-logs : View NetBox server logs"; echo ""; echo "⚙️ Background Jobs (RQ Worker):"; echo " rq-status : Check if RQ worker is running"; echo " rq-logs : View RQ worker logs"; echo " rq-stats : Show RQ queue statistics"; echo " rq-jobs : List jobs in default queue"; echo " rq-failed : List failed jobs"; echo " rq-recent : Show recent NetBox jobs"; echo ""; echo "🛠️ Development Tools:"; echo " netbox-shell : Open NetBox Django shell"; echo " netbox-test : Run plugin tests"; echo " netbox-manage : Run Django management commands"; echo " plugin-install : Reinstall plugin in development mode"; echo ""; echo "🧹 Code Quality:"; echo " ruff-check : Check code with Ruff"; echo " ruff-format : Format code with Ruff"; echo " ruff-fix : Auto-fix code issues with Ruff"; echo ""; echo "🔎 Diagnostics:"; echo " diagnose : Run startup diagnostics"; echo " dev-help : Show this help message"; echo ""; echo "📖 NetBox available at: http://localhost:8000 (admin/admin)"; echo ""'
+dev-help() {
+ cat <<'EOF'
+🎯 NetBox LibreNMS Plugin Development Commands:
+
+📊 NetBox Server Management:
+ netbox-run-bg : Start NetBox in background
+ netbox-run : Start NetBox in foreground (for debugging)
+ netbox-stop : Stop NetBox and RQ worker
+ netbox-restart : Restart NetBox and RQ worker
+ netbox-reload : Reinstall plugin and restart NetBox
+ netbox-status : Check if NetBox and RQ worker are running
+ netbox-logs : View NetBox server logs
+
+⚙️ Background Jobs (RQ Worker):
+ rq-status : Check if RQ worker is running
+ rq-logs : View RQ worker logs
+ rq-stats : Show RQ queue statistics
+ rq-jobs : List jobs in default queue
+ rq-failed : List failed jobs
+ rq-recent : Show recent NetBox jobs
+
+🛠️ Development Tools:
+ netbox-shell : Open NetBox Django shell
+ netbox-test : Run plugin tests
+ netbox-manage : Run Django management commands
+ plugin-install : Reinstall plugin in development mode
+
+🧹 Code Quality:
+ ruff-check : Check code with Ruff
+ ruff-format : Format code with Ruff
+ ruff-fix : Auto-fix code issues with Ruff
+
+🔎 Diagnostics:
+ diagnose : Run startup diagnostics
+ dev-help : Show this help message
+
+📖 NetBox available at: http://localhost:8000 (admin/admin)
+EOF
+}📝 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.
| # Help | |
| alias dev-help='echo "🎯 NetBox LibreNMS Plugin Development Commands:"; echo ""; echo "📊 NetBox Server Management:"; echo " netbox-run-bg : Start NetBox in background"; echo " netbox-run : Start NetBox in foreground (for debugging)"; echo " netbox-stop : Stop NetBox and RQ worker"; echo " netbox-restart : Restart NetBox and RQ worker"; echo " netbox-reload : Reinstall plugin and restart NetBox"; echo " netbox-status : Check if NetBox and RQ worker are running"; echo " netbox-logs : View NetBox server logs"; echo ""; echo "⚙️ Background Jobs (RQ Worker):"; echo " rq-status : Check if RQ worker is running"; echo " rq-logs : View RQ worker logs"; echo " rq-stats : Show RQ queue statistics"; echo " rq-jobs : List jobs in default queue"; echo " rq-failed : List failed jobs"; echo " rq-recent : Show recent NetBox jobs"; echo ""; echo "🛠️ Development Tools:"; echo " netbox-shell : Open NetBox Django shell"; echo " netbox-test : Run plugin tests"; echo " netbox-manage : Run Django management commands"; echo " plugin-install : Reinstall plugin in development mode"; echo ""; echo "🧹 Code Quality:"; echo " ruff-check : Check code with Ruff"; echo " ruff-format : Format code with Ruff"; echo " ruff-fix : Auto-fix code issues with Ruff"; echo ""; echo "🔎 Diagnostics:"; echo " diagnose : Run startup diagnostics"; echo " dev-help : Show this help message"; echo ""; echo "📖 NetBox available at: http://localhost:8000 (admin/admin)"; echo ""' | |
| # Help | |
| dev-help() { | |
| cat <<'EOF' | |
| 🎯 NetBox LibreNMS Plugin Development Commands: | |
| 📊 NetBox Server Management: | |
| netbox-run-bg : Start NetBox in background | |
| netbox-run : Start NetBox in foreground (for debugging) | |
| netbox-stop : Stop NetBox and RQ worker | |
| netbox-restart : Restart NetBox and RQ worker | |
| netbox-reload : Reinstall plugin and restart NetBox | |
| netbox-status : Check if NetBox and RQ worker are running | |
| netbox-logs : View NetBox server logs | |
| ⚙️ Background Jobs (RQ Worker): | |
| rq-status : Check if RQ worker is running | |
| rq-logs : View RQ worker logs | |
| rq-stats : Show RQ queue statistics | |
| rq-jobs : List jobs in default queue | |
| rq-failed : List failed jobs | |
| rq-recent : Show recent NetBox jobs | |
| 🛠️ Development Tools: | |
| netbox-shell : Open NetBox Django shell | |
| netbox-test : Run plugin tests | |
| netbox-manage : Run Django management commands | |
| plugin-install : Reinstall plugin in development mode | |
| 🧹 Code Quality: | |
| ruff-check : Check code with Ruff | |
| ruff-format : Format code with Ruff | |
| ruff-fix : Auto-fix code issues with Ruff | |
| 🔎 Diagnostics: | |
| diagnose : Run startup diagnostics | |
| dev-help : Show this help message | |
| 📖 NetBox available at: http://localhost:8000 (admin/admin) | |
| EOF | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/scripts/load-aliases.sh around lines 64 - 65, Replace the long
single-line alias `dev-help` with a shell function named `dev-help` that prints
the same multi-line help text; specifically, remove the alias definition and add
a function `dev-help() { ... }` that emits the formatted help (use a heredoc or
multiple echo lines) so the help is readable and maintainable, and ensure the
function is defined in the same script so it’s available when the script is
sourced.
| ## Permission System | ||
| - Uses two-tier permissions via `LibreNMSSettings` model: `view_librenmssettings` (read) and `change_librenmssettings` (write). See `docs/development/permissions.md`. | ||
| - All views inherit `LibreNMSPermissionMixin` from `views/mixins.py`. Permission constants live in `constants.py`. | ||
| - **Sync POST handlers** must call `require_write_permission()` at the start and return early if it returns a response. | ||
| - `require_write_permission()` handles HTMX requests with `HX-Redirect` header; regular requests get standard redirect. | ||
| - API endpoints use `LibreNMSPluginPermission` class in `api/views.py` (GET=view, others=change). | ||
| - Navigation menu permissions are set in `navigation.py` using permission constants. | ||
| - **Background job polling requires superuser** (NetBox core restriction on `/api/core/background-tasks/`). Non-superusers automatically fall back to synchronous mode—see `should_use_background_job()` methods. | ||
|
|
There was a problem hiding this comment.
Missing blank line before heading (MD022).
The ## Permission System heading on Line 29 needs a blank line above it (after the preceding section's last line), and ## When in Doubt on Line 38 needs a blank line above it (after Line 37). The linter flags both.
Proposed fix
+
## Permission Systemand
- **Background job polling requires superuser** (NetBox core restriction on `/api/core/background-tasks/`). Non-superusers automatically fall back to synchronous mode—see `should_use_background_job()` methods.
+
## When in Doubt📝 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.
| ## Permission System | |
| - Uses two-tier permissions via `LibreNMSSettings` model: `view_librenmssettings` (read) and `change_librenmssettings` (write). See `docs/development/permissions.md`. | |
| - All views inherit `LibreNMSPermissionMixin` from `views/mixins.py`. Permission constants live in `constants.py`. | |
| - **Sync POST handlers** must call `require_write_permission()` at the start and return early if it returns a response. | |
| - `require_write_permission()` handles HTMX requests with `HX-Redirect` header; regular requests get standard redirect. | |
| - API endpoints use `LibreNMSPluginPermission` class in `api/views.py` (GET=view, others=change). | |
| - Navigation menu permissions are set in `navigation.py` using permission constants. | |
| - **Background job polling requires superuser** (NetBox core restriction on `/api/core/background-tasks/`). Non-superusers automatically fall back to synchronous mode—see `should_use_background_job()` methods. | |
| ## Permission System | |
| - Uses two-tier permissions via `LibreNMSSettings` model: `view_librenmssettings` (read) and `change_librenmssettings` (write). See `docs/development/permissions.md`. | |
| - All views inherit `LibreNMSPermissionMixin` from `views/mixins.py`. Permission constants live in `constants.py`. | |
| - **Sync POST handlers** must call `require_write_permission()` at the start and return early if it returns a response. | |
| - `require_write_permission()` handles HTMX requests with `HX-Redirect` header; regular requests get standard redirect. | |
| - API endpoints use `LibreNMSPluginPermission` class in `api/views.py` (GET=view, others=change). | |
| - Navigation menu permissions are set in `navigation.py` using permission constants. | |
| - **Background job polling requires superuser** (NetBox core restriction on `/api/core/background-tasks/`). Non-superusers automatically fall back to synchronous mode—see `should_use_background_job()` methods. | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 29-29: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/copilot-instructions.md around lines 29 - 37, Add a blank line
immediately before the "## Permission System" heading and another blank line
immediately before the "## When in Doubt" heading in
.github/copilot-instructions.md so each top-level section is separated by an
empty line; specifically insert an empty line after the preceding paragraph
ending before "## Permission System" and after line 37 so "## When in Doubt" is
preceded by a blank line.
| ## Superuser Requirement for Background Jobs | ||
| - NetBox's `/api/core/background-tasks/` endpoint requires **superuser** (`IsSuperuser` in `BaseRQViewSet`). | ||
| - Non-superuser users cannot poll job status; they get 403 Forbidden. | ||
| - The plugin automatically falls back to synchronous mode for non-superusers—see `should_use_background_job()` in `list.py` and `actions.py`. | ||
| - This is a NetBox core design decision, not a plugin limitation. No amount of permissions (including `core.view_job`) bypasses it. | ||
|
|
There was a problem hiding this comment.
Fix markdown heading spacing to satisfy linter.
The markdownlint tool flags a missing blank line around the heading. Ensure there's a blank line after line 35 (before the next section) to maintain consistent formatting with the rest of the document and satisfy the MD022 rule.
📝 Suggested spacing adjustment
Ensure a blank line exists after line 35:
- This is a NetBox core design decision, not a plugin limitation. No amount of permissions (including `core.view_job`) bypasses it.
+
## Custom Sync EndpointNote: The content itself is valuable—documenting the superuser requirement and synchronous fallback behavior provides important context for developers working with background jobs.
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 30-30: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/instructions/background-jobs.instructions.md around lines 30 - 35,
Add a blank line after the "Superuser Requirement for Background Jobs" heading
block to satisfy markdownlint MD022; update the section following the paragraph
that references BaseRQViewSet, IsSuperuser, and the should_use_background_job()
helpers in list.py and actions.py so there is an empty line separating the
heading from the next section/content.
| ## JavaScript Fetch Patterns | ||
| - Always check `response.ok` before processing fetch responses to catch HTTP errors. | ||
| - In catch blocks, show `error.message` for debugging rather than generic messages. | ||
| - The import filter form uses fetch with `Accept: application/json, text/html`—JSON for background jobs, HTML for synchronous mode. |
There was a problem hiding this comment.
Missing blank line after heading (MD022).
The ## JavaScript Fetch Patterns heading needs a blank line after it before the first bullet.
Proposed fix
## JavaScript Fetch Patterns
+
- Always check `response.ok` before processing fetch responses to catch HTTP errors.📝 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.
| ## JavaScript Fetch Patterns | |
| - Always check `response.ok` before processing fetch responses to catch HTTP errors. | |
| - In catch blocks, show `error.message` for debugging rather than generic messages. | |
| - The import filter form uses fetch with `Accept: application/json, text/html`—JSON for background jobs, HTML for synchronous mode. | |
| ## JavaScript Fetch Patterns | |
| - Always check `response.ok` before processing fetch responses to catch HTTP errors. | |
| - In catch blocks, show `error.message` for debugging rather than generic messages. | |
| - The import filter form uses fetch with `Accept: application/json, text/html`—JSON for background jobs, HTML for synchronous mode. |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/instructions/frontend.instructions.md around lines 19 - 22, Add a
blank line immediately after the heading "## JavaScript Fetch Patterns" to
satisfy MD022; open the .github/instructions/frontend.instructions.md content
around the "## JavaScript Fetch Patterns" heading and insert one empty line
before the first bullet so the heading is separated from the list.
| def check_existing_cable(self, local_interface, remote_interface): | ||
| """Return True if a cable already exists for either interface.""" | ||
| return Cable.objects.filter( | ||
| Q(terminations__termination_id=local_interface.pk) | Q(terminations__termination_id=remote_interface.pk) | ||
| ).exists() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "cables.py" | head -20Repository: marcinpsk/netbox-librenms-plugin
Length of output: 163
🏁 Script executed:
git ls-files | grep -E "views/sync|cables" | head -20Repository: marcinpsk/netbox-librenms-plugin
Length of output: 495
🏁 Script executed:
cat netbox_librenms_plugin/views/sync/cables.py | head -80Repository: marcinpsk/netbox-librenms-plugin
Length of output: 3294
🌐 Web query:
NetBox Cable CableTermination model termination_type termination_id
💡 Result:
In NetBox, dcim.CableTermination uses a generic relation to point at “whatever object is terminated by this cable end” (interface, front/rear port, power port/outlet, circuit termination, etc.).
termination_type: the target object’s ContentType expressed as an"app_label.model"string (e.g."dcim.interface","circuits.circuittermination"). This is the same pattern NetBox uses generally for “content type + primary key” global identification. [1]termination_id: the target object’s primary key (integer ID) within that model. [2]- Together they identify the terminated object (i.e., effectively
termination = GenericForeignKey(termination_type, termination_id)). NetBox also exposes a resolvedterminationobject in API responses. [2]
Related note (often the source of confusion): starting with NetBox v3.3, the Cable REST API stopped using termination_a_type/termination_a_id and instead uses a_terminations / b_terminations lists of {object_type, object_id}. [3]
Sources: [1] NetBox dev docs on content type + PK identifiers, [2] NetBox issue showing CableTermination API fields termination_type/termination_id and resolved termination, [3] NetBox v3.3 release notes on cable termination API changes.
🏁 Script executed:
rg -t py "CableTermination|termination_type|termination_id" --max-count=15 -A 2Repository: marcinpsk/netbox-librenms-plugin
Length of output: 344
🏁 Script executed:
cat netbox_librenms_plugin/views/sync/cables.py | sed -n '40,100p'Repository: marcinpsk/netbox-librenms-plugin
Length of output: 2683
🏁 Script executed:
rg -t py "Cable.objects.create|a_terminations|b_terminations" netbox_librenms_plugin/views/sync/cables.py -A 3 -B 3Repository: marcinpsk/netbox-librenms-plugin
Length of output: 478
🏁 Script executed:
rg -t py "terminations__" netbox_librenms_plugin/ -B 2 -A 2Repository: marcinpsk/netbox-librenms-plugin
Length of output: 543
🏁 Script executed:
rg -t py "ContentType" netbox_librenms_plugin/ -B 1 -A 1Repository: marcinpsk/netbox-librenms-plugin
Length of output: 3725
🏁 Script executed:
find . -type f -name "*.py" -path "*/tests/*" | xargs grep -l "check_existing_cable\|Cable" 2>/dev/null | head -5Repository: marcinpsk/netbox-librenms-plugin
Length of output: 58
🏁 Script executed:
find . -type f -name "*.py" -path "*/tests/*" | xargs grep -l "cables\|check_existing" 2>/dev/nullRepository: marcinpsk/netbox-librenms-plugin
Length of output: 130
🏁 Script executed:
rg -t py "Cable.objects.filter\|CableTermination" -B 3 -A 3Repository: marcinpsk/netbox-librenms-plugin
Length of output: 58
🌐 Web query:
NetBox Cable CableTermination ContentType filter query django ORM
💡 Result:
In NetBox, dcim.CableTermination stores its endpoint as a Django ContentType FK (termination_type) plus an object PK (termination_id). (github.com)
So in Django ORM you filter by termination_type (or termination_type_id) like this:
from django.contrib.contenttypes.models import ContentType
from dcim.models import CableTermination, Interface # adjust import path for your NetBox version
ct = ContentType.objects.get_for_model(Interface) # ContentType for "dcim.interface"
qs = CableTermination.objects.filter(termination_type=ct)
# or: CableTermination.objects.filter(termination_type_id=ct.pk)
# example: only “B” end terminations that are Interfaces
qs_b = CableTermination.objects.filter(cable_end="B", termination_type=ct)NetBox uses Django’s content types framework (app_label + model) to identify models (e.g. dcim.interface). (netbox.hsbne.org)
Citations:
- 1: Updating a Cable Termination over the API results in a missing connection/trace netbox-community/netbox#15390
- 2: https://netbox.hsbne.org/static/docs/development/models/?utm_source=openai
Scope duplicate-cable detection by termination type to prevent false matches.
Filtering by termination_id alone matches any termination object with that ID across all types (Interface, PowerPort, FrontPort, etc.), falsely skipping cable creation if another termination type shares the same ID. Since this code only creates Interface cables, constrain the query to Interface terminations.
🔧 Suggested fix
+from django.contrib.contenttypes.models import ContentType
@@
def check_existing_cable(self, local_interface, remote_interface):
"""Return True if a cable already exists for either interface."""
+ interface_ct = ContentType.objects.get_for_model(Interface)
- return Cable.objects.filter(
- Q(terminations__termination_id=local_interface.pk) | Q(terminations__termination_id=remote_interface.pk)
- ).exists()
+ return Cable.objects.filter(
+ terminations__termination_type=interface_ct,
+ terminations__termination_id__in=[local_interface.pk, remote_interface.pk],
+ ).exists()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/cables.py` around lines 55 - 59, The
current check_existing_cable method (check_existing_cable) only filters by
terminations__termination_id and therefore can match other termination types;
restrict the search to Interface terminations by adding the termination type
check to the query (e.g. include
terminations__termination_type__model='interface' in each Q clause or otherwise
constrain terminations__termination_type to the Interface ContentType) so that
Cable.objects.filter(...) only considers Interface terminations when checking
for existing cables between local_interface and remote_interface.
| def post(self, request, pk): | ||
| """Sync the device name from LibreNMS sysName.""" | ||
| if error := self.require_all_permissions("POST"): | ||
| return error | ||
|
|
||
|
|
||
| device = get_object_or_404(Device, pk=pk) |
There was a problem hiding this comment.
Fix formatting: extra blank line causing ruff failure.
The pipeline reports ruff format --check failure for this file. There's a double blank line (lines 23–24) inside the method body that needs to be collapsed to a single blank line.
🛠️ Proposed fix
def post(self, request, pk):
"""Sync the device name from LibreNMS sysName."""
if error := self.require_all_permissions("POST"):
return error
-
device = get_object_or_404(Device, pk=pk)📝 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.
| def post(self, request, pk): | |
| """Sync the device name from LibreNMS sysName.""" | |
| if error := self.require_all_permissions("POST"): | |
| return error | |
| device = get_object_or_404(Device, pk=pk) | |
| def post(self, request, pk): | |
| """Sync the device name from LibreNMS sysName.""" | |
| if error := self.require_all_permissions("POST"): | |
| return error | |
| device = get_object_or_404(Device, pk=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 19 - 25, The
method post has an extra blank line causing ruff format to fail; inside the post
method (function post) collapse the double blank line between the
require_all_permissions check (require_all_permissions) and the Device retrieval
(get_object_or_404(Device, pk=pk)) into a single blank line so the body has only
one blank line there and matches project formatting rules.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/lint-format.yaml (1)
30-34: 🧹 Nitpick | 🔵 TrivialRemoving
continue-on-erroris a positive strictness improvement.Lint and format violations will now correctly fail the workflow. However,
ruffis installed without a pinned version, which means CI behavior can silently change when a new Ruff release introduces new rules or formatting changes.📌 Suggested: pin the ruff version
- pip install ruff + pip install ruff==0.9.7(Replace with whatever version the team currently uses locally.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint-format.yaml around lines 30 - 34, The workflow runs "Run Ruff linting" and "Run Ruff formatting check" without a pinned ruff version; add a step or change those steps so Ruff is installed at a specific pinned version (e.g., ruff==<team-version>) before invoking ruff check and ruff format --check, ensuring the CI uses a deterministic Ruff release; reference the "Run Ruff linting" and "Run Ruff formatting check" steps when making the change.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/lint-format.yaml:
- Around line 30-34: The workflow runs "Run Ruff linting" and "Run Ruff
formatting check" without a pinned ruff version; add a step or change those
steps so Ruff is installed at a specific pinned version (e.g.,
ruff==<team-version>) before invoking ruff check and ruff format --check,
ensuring the CI uses a deterministic Ruff release; reference the "Run Ruff
linting" and "Run Ruff formatting check" steps when making the change.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/lint-format.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
🔇 Additional comments (1)
.github/workflows/lint-format.yaml (1)
17-23: Good updates to action versions and Python runtime.Bumping
actions/checkoutto v4,actions/setup-pythonto v5, and Python to 3.12 are all sensible modernizations.
…provements
Add serial-based device matching to the import pipeline, a full conflict
resolution UI, per-user toggle persistence, device field sync from the
import modal, and multiple safety/bug fixes across views and JS.
Serial Number Matching
- Serial number as a blocking match criterion (checked between hostname
and IP), giving hardware-identity priority over network-layer matches.
- Serial drift detection on devices already linked by librenms_id: flags
update_serial when serials diverge, or conflict when the incoming
serial belongs to another NetBox device.
- serial_confirmed flag set when linked device serial matches LibreNMS.
- serial_duplicate flag distinguishes true duplicates (incoming serial on
another device) from devices found by serial match.
- serial_action semantics: None | link | conflict | update_serial |
hostname_differs.
Conflict Resolution
- DeviceConflictActionView (HTMX POST) with actions: link, update,
update_serial, sync_name, sync_serial, sync_platform, sync_device_type,
update_type.
- Serial ownership checks on update/sync actions return 409 when the
incoming serial is already assigned to a different NetBox device.
- Device type mismatch detection: warns when existing device type differs
from LibreNMS hardware; requires force checkbox to proceed.
- Conflict/Details button in import table: red for type mismatch, yellow
for serial/hostname conflicts, blue for info-only.
- Modal closes after successful action via HX-Trigger: closeModal.
Import Validation Modal
- Redesigned two-column layout (LibreNMS Status + Device Info table).
- Inline sync buttons for name, serial, platform, and device type.
- Badge-style status indicators: Linked, Name match/differs, Serial
confirmed/differs, Type mismatch.
- _build_sync_info() computes comparison data between LibreNMS device
and existing NetBox device (serial, platform, device type).
- Modal widened from modal-lg to modal-xl.
Device Field Sync Views
- UpdateDeviceNameView: sync NetBox device name from LibreNMS sysName.
- name_sync_available / suggested_name flags for linked devices whose
name differs from sysName (Device only; VMs intentionally excluded
since UpdateDeviceNameView does not support VM objects).
- _refresh_existing_device() re-fetches cached devices from DB so that
role/name/type changes in NetBox are reflected immediately; recomputes
readiness with correct VM branching (site+role for VMs, site+type+role
for devices) using .get("found") consistently.
Toggle & Preference Persistence
- SaveUserPrefView endpoint for persisting toggle state via JS fetch.
- use_sysname, strip_domain, and interface_name_field persist per-user
via NetBox user.config across page reloads.
- Import page reads user prefs with fallback chain: request param →
user pref → LibreNMSSettings model → plugin config.
- Settings page save also updates current user preferences.
Safety & Bug Fixes
- Fix critical bug: enabled was set after interface.save() and never
persisted; moved into update_interface_attributes before save().
- Add full_clean()/try-except to all device field update views
(serial, type, platform, create-platform, VC serial, name) with
proper rollback of the original value on failure.
- Use address__net_host instead of address__startswith for exact IP
matching (prevents false positives with overlapping prefixes).
- Handle DoesNotExist/ValueError/TypeError for user-submitted
selected_device_id in interface sync (fallback to obj).
- Use .get() for ifSpeed/ifType to prevent KeyError.
- Guard None coordinates in create_librenms_location.
- Remove unreachable dead code in locations.py q-filtering.
- Stabilize VC member ordering with order_by("vc_position", "name").
- Fix wrong reverse URL (vm_interface_sync → device_interface_sync).
- Fix </th> → </td> tag mismatch in librenms_sync_base.html.
- Wrap Platform.objects.create in try/except for slug collision.
- Initialize name_sync_available and suggested_name in result dict.
- Remove redundant pass after logger.error.
JavaScript
- hideModal: recover existing Bootstrap getInstance before falling back
to manual close; cache filter-processing-modal element once at top of
pollJobStatus instead of 9 repeated getElementById calls.
- Use hideModal() consistently for results modal auto-close.
- CSRF fallback to csrftoken cookie when HTMX swaps remove hidden input.
- interface_name_field pref saved to user.config via fetch on change.
- WONTFIX comment on fallbackBackdropRef (single-modal, Tabler env).
Docstrings & Templates
- ~130 docstrings across 26 files (62.6% → 98.9% coverage).
- Sync page: empty state messages on tabs before data is loaded.
- Sync page: Name row with sync button in Device Information table.
- Sync page: device type sync button styled as btn-outline-danger.
- Compute existing_device_url once for VM-safe links in modal template.
Tests (1071 new lines, 280 total tests passing)
- 12 serial matching tests (by serial, hostname+serial drift, librenms_id
serial drift, duplicate detection, VM exclusion).
- 8 conflict resolution action tests (link, update, update_serial,
sync_name, sync_platform, sync_device_type).
- 3 toggle persistence tests.
- _build_sync_info comparison tests.
- Device type mismatch and serial confirmation tests.
- Refactored TestSerialNumberMatching to setup_method/teardown_method.
Devcontainer
- Fix MITM proxy SSL for pre-commit (CA bundle cert splitting).
- Fix aliases not loading in postAttach terminal.
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 (13)
netbox_librenms_plugin/views/status_check.py (1)
52-58: 🧹 Nitpick | 🔵 TrivialPre-existing N+1 API calls: consider batching in a follow-up.
Both
get_querysetmethods callself.librenms_api.get_librenms_id()individually per device/VM inside a loop. For large filtered querysets this will be slow due to one API (or DB) roundtrip per object. Not introduced by this PR, but worth noting for a future optimization pass — a batch lookup would significantly reduce latency.Also applies to: 98-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/status_check.py` around lines 52 - 58, The loop in get_queryset (and the similar VM method) calls self.librenms_api.get_librenms_id(device) per object causing N+1 API calls; change this to a batched lookup by adding a bulk method on librenms_api (e.g., get_librenms_ids_for_devices or get_librenms_id_map) that accepts a list of devices or IDs and returns a mapping of device.pk -> librenms_id, then replace the per-device calls in get_queryset and the VM variant to call that bulk method once and populate device_status_map[device.pk] = bool(mapping.get(device.pk)). Ensure you update references to librenms_api.get_librenms_id to use the new bulk method name and handle missing keys safely.netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (1)
796-807: 🧹 Nitpick | 🔵 TrivialMinor: generic
alert()may expose raw HTTP status text to users.When the thrown error from line 754 reaches here,
error.messagewill be something like"HTTP 500: Internal Server Error". This is fine for developers but may confuse end users. Consider a user-friendly fallback message while logging the raw error for debugging.♻️ Suggested improvement
if (error.name === 'AbortError') { // Request was cancelled by user - silent } else { console.error('Error fetching filtered results:', error); - // Show more specific error if available - const errorMsg = error.message || 'Error loading filtered results. Please try again.'; - alert(errorMsg); + alert('Error loading filtered results. Please try again.'); }🤖 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_import.js` around lines 796 - 807, The current error handler alerts raw error.message (e.g. "HTTP 500: Internal Server Error") which may confuse users; change the alert to show a friendly fallback (e.g. "An error occurred while loading results. Please try again.") while retaining console.error(error) for debugging. Concretely, in the catch block that references error, errorMsg, filterModalManager.hide(), and currentAbortController = null, compute a userMessage = 'An error occurred while loading results. Please try again.' and call alert(userMessage) instead of alert(errorMsg), but keep console.error('Error fetching filtered results:', error) so the raw error is still logged..devcontainer/scripts/setup.sh (1)
281-295: 🧹 Nitpick | 🔵 TrivialSilently converting the user's Git remote from SSH to HTTPS may surprise users who prefer SSH.
Consider logging a more prominent notice or making this opt-out (e.g., via an env var), since some users may have SSH keys configured and prefer that transport even inside containers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/scripts/setup.sh around lines 281 - 295, Currently the script silently rewrites SSH Git remotes to HTTPS; update it to respect an opt-out env var (e.g., PLUGIN_PRESERVE_GIT_SSH) and emit a more prominent notice before changing remotes: check PLUGIN_PRESERVE_GIT_SSH (or similar) and skip the conversion if set, and when performing the change (the block that reads CURRENT_REMOTE, computes HTTPS_URL and runs git remote set-url origin) print a clear, visible warning to the user and instructions on how to opt out (and log the original and new URL); keep the existing behavior when the env var is not set.netbox_librenms_plugin/views/sync/cables.py (3)
44-53:⚠️ Potential issue | 🔴 CriticalBug:
create_cableswallows exceptions, causinghandle_cable_creationto report false success.
create_cablecatches all exceptions and flashes an error message but returnsNone.handle_cable_creationthen unconditionally returns{"status": "valid"}on Line 107, so a failed cable creation is counted as successful in the results summary.Either propagate the failure (e.g., return a boolean from
create_cable) or catch the failure inhandle_cable_creation.Proposed fix
def create_cable(self, local_interface, remote_interface, request): """Create a cable between local and remote interfaces.""" try: Cable.objects.create( a_terminations=[local_interface], b_terminations=[remote_interface], status="connected", ) + return True except Exception as exc: # pragma: no cover - protects UX messages.error(request, f"Failed to create cable: {str(exc)}") + return False ... def handle_cable_creation(self, link_data, interface): """Create a cable from link data and return the operation result.""" if not self.verify_cable_creation_requirements(link_data): return {"status": "invalid", "interface": interface["interface"]} try: local_interface = Interface.objects.get(pk=link_data["netbox_local_interface_id"]) remote_interface = Interface.objects.get(pk=link_data["netbox_remote_interface_id"]) if self.check_existing_cable(local_interface, remote_interface): return {"status": "duplicate", "interface": interface["interface"]} - self.create_cable(local_interface, remote_interface, self.request) - return {"status": "valid", "interface": interface["interface"]} + if self.create_cable(local_interface, remote_interface, self.request): + return {"status": "valid", "interface": interface["interface"]} + return {"status": "invalid", "interface": interface["interface"]} except Interface.DoesNotExist: return {"status": "missing_remote", "interface": interface["interface"]}Also applies to: 94-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 44 - 53, The create_cable function currently swallows exceptions and returns None, so change create_cable (in netbox_librenms_plugin.views.sync.cables) to return an explicit success flag (True on successful Cable.objects.create, False on exception) or re-raise the exception; keep the messages.error call but return False in the except block. Then update handle_cable_creation to check the return value from create_cable and only set {"status": "valid"} when create_cable returns True (otherwise set an error/failed status and include the error context), ensuring callers rely on the boolean instead of assuming success.
112-121:⚠️ Potential issue | 🟠 Major
transaction.atomic()is undermined by the internal exception handling increate_cable.
create_cablecatches exceptions at Line 52, which prevents the exception from propagating to thetransaction.atomic()block. This means a failedCable.objects.createwon't trigger a rollback — previously successful cables in the same batch will be committed while the failed one is silently lost, and its status is reported as "valid" (per the bug above).If the intent is all-or-nothing batch creation, the exception must propagate. If partial success is acceptable, remove
transaction.atomic()and ensure each cable's result is reported accurately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 112 - 121, process_interface_sync is wrapped in transaction.atomic but create_cable swallows exceptions internally, preventing rollbacks; either remove the internal try/except in create_cable (so exceptions from Cable.objects.create propagate and atomic can rollback) or, if partial success is desired, remove transaction.atomic from process_interface_sync and update process_single_interface/create_cable to return explicit failure statuses (e.g., "invalid" or "duplicate") for any caught errors; locate create_cable and process_interface_sync/process_single_interface to implement the chosen approach and ensure results[result["status"]] correctly reflects failures when exceptions occur.
23-35: 🧹 Nitpick | 🔵 TrivialRemove unused
device_idfrom interface collection.The
device_idcollected on line 32–33 is never accessed downstream. The interface dict is passed throughprocess_single_interface→handle_cable_creation, but onlyinterface["interface"](the port name) is used. The cable creation pulls device IDs fromlink_datainstead. Remove thedevice_idkey to eliminate confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 23 - 35, The get_selected_interfaces function is collecting a device_id into each interface dict that is never used downstream; update get_selected_interfaces to only append the interface name (e.g., {"interface": interface} or even just the string if callers accept it) and remove the device_id key/assignment and the request.POST.get(f"device_selection_{interface}") lookup; ensure callers (process_single_interface and handle_cable_creation) continue to access interface["interface"] (or adapt them if you switch to a plain string) and that link_data remains the source of device IDs for cable creation.netbox_librenms_plugin/tables/device_status.py (3)
262-262: 🧹 Nitpick | 🔵 TrivialRedundant local import —
reverseis already imported at file level (Line 6).The same
from django.urls import reverseappears as a local import insiderender_netbox_cluster(Line 262),render_netbox_role(Line 329), andrender_netbox_rack(Line 403). Remove these sincereverseis already available at module scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/device_status.py` at line 262, Remove the redundant local imports of reverse inside the helper functions render_netbox_cluster, render_netbox_role, and render_netbox_rack and rely on the module-level import already present; edit each function to delete the line "from django.urls import reverse" so there are no duplicate imports while preserving existing uses of reverse in those functions.
94-108: 🧹 Nitpick | 🔵 TrivialCaching all clusters and roles on every table instantiation.
Lines 102–103 load all
ClusterandDeviceRoleobjects into memory on each table construction. For large deployments this could be costly. Consider lazy loading or limiting the queryset if the table only needs a subset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/device_status.py` around lines 94 - 108, The constructor (__init__) eagerly loads all Cluster and DeviceRole records into self._cached_clusters and self._cached_roles which can blow memory; change this to lazy loading by replacing the immediate list() calls with deferred access (e.g., use a cached_property or getter) so the DB is only hit when _cached_clusters/_cached_roles are first accessed, or restrict the query to only the items needed for the current table (e.g., filter by IDs present in the table data or use values_list to fetch only names/ids). Update references to these attributes to call the lazy getter (or property) and keep the sorting call to _sort_data() intact so behavior is unchanged.
138-144: 🛠️ Refactor suggestion | 🟠 MajorAvoid directly modifying
self.data.data— sort data before table construction instead.
self.data.data.sort(...)accesses the internalTableDatastructure, which django-tables2 explicitly marks as internal API subject to change. The defensive try/except doesn't address the core fragility. Since the data being sorted is a list of dictionaries (not QuerySets), sort it in the view before passing it to the table constructor, then pass the pre-sorted data toDeviceImportTable. This follows django-tables2 best practices and eliminates reliance on internal attributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tables/device_status.py` around lines 138 - 144, The code currently mutates the internal django-tables2 structure by calling self.data.data.sort(...) inside DeviceImportTable; instead, remove that mutation and sort the raw list of dicts in the view before constructing DeviceImportTable. Locate where the view builds the table (passes data into DeviceImportTable) and, when the dataset is a list, call sorted(data, key=sort_key, reverse=reverse) and pass that sorted list as the data argument to DeviceImportTable; also delete or revert the try/except block that references self.data.data and self.data so the table class no longer touches internal attributes.netbox_librenms_plugin/views/settings_views.py (1)
132-183:⚠️ Potential issue | 🟠 MajorUnescaped user-controlled/external data rendered as HTML — XSS risk.
Exception messages (
str(e)) on Lines 158, 174, 181 and LibreNMS API response values (version,database,php_version) on Lines 148–150 are interpolated directly into HTML via f-strings. If LibreNMS returns malicious payloads or exception messages contain HTML/JS, this is exploitable as stored/reflected XSS.Use
django.utils.html.escape()on all externally sourced values before embedding them in HTML responses.Proposed fix (example for one block)
+from django.utils.html import escape + # In the success block: - version = system_info.get("local_ver", "Unknown") - database = system_info.get("database_ver", "Unknown") - php_version = system_info.get("php_ver", "Unknown") + version = escape(system_info.get("local_ver", "Unknown")) + database = escape(system_info.get("database_ver", "Unknown")) + php_version = escape(system_info.get("php_ver", "Unknown"))Apply the same
escape()treatment toerror_msgandstr(e)in all the exception/error response blocks below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/settings_views.py` around lines 132 - 183, The response HTML is currently embedding unescaped external data (LibreNMS API fields: version, database, php_version; API error_msg; and exception strings from except blocks) which creates an XSS risk; update the view in settings_views.py to import django.utils.html.escape and wrap all externally sourced values (version, database, php_version, error_msg, and str(e) in the ValueError and general Exception handlers) with escape() before interpolating into the HttpResponse f-strings (keep the existing success/failure HTML structure and LibreNMSAPI/test_connection usage).netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
213-216:⚠️ Potential issue | 🟡 MinorStray
{{ member.name }}text node outside<option>tag.Line 214 renders
{{ member.name }}as bare text before the<option>element, causing member names to leak into the modal body as unstructured text. This is pre-existing but worth fixing.🛠️ Proposed fix
{% for member in interface_sync.virtual_chassis_members %} - {{ member.name }} <option value="{{ member.id }}">{{ member.name }}</option> {% endfor %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html` around lines 213 - 216, The template loop over interface_sync.virtual_chassis_members currently outputs a stray text node "{{ member.name }}" before the option, causing unstructured names to appear; remove that standalone "{{ member.name }}" (or move it inside the <option> if intended) so only the <option value="{{ member.id }}">{{ member.name }}</option> remains—adjust the loop around interface_sync.virtual_chassis_members and references to member.name and member.id to eliminate the duplicate/bare output.netbox_librenms_plugin/views/sync/interfaces.py (2)
297-302:⚠️ Potential issue | 🟡 MinorBare
except Exceptioninside the loop silently continues on unexpected errors.Lines 300-302 catch all exceptions, including programming errors (e.g.,
AttributeError,IntegrityError), log them as simple error strings, and continue. Inside atransaction.atomic()block, some exceptions (likeIntegrityError) would have already broken the savepoint, making subsequent operations fail withTransactionManagementError. The# pragma: no cover - defensivesuggests this is intentional, but be aware that swallowingIntegrityErrorinside an atomic block is problematic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 297 - 302, The current broad except in the loop (after catching Interface.DoesNotExist and VMInterface.DoesNotExist) swallows all exceptions; narrow this by removing the bare `except Exception` and explicitly handling expected runtime errors (e.g., ValueError/KeyError) while re-raising or allowing Django DB errors to propagate: catch and append messages for non-fatal exceptions only, and explicitly `raise` for IntegrityError, TransactionManagementError, DatabaseError (or re-raise any exception subclass from django.db) so they break the transaction; reference the same block where Interface.DoesNotExist and VMInterface.DoesNotExist are handled and the `errors` list is appended to, and ensure any defensive logging preserves the original exception when re-raising.
160-170:⚠️ Potential issue | 🟡 MinorUse
get_or_create()to prevent duplicate MAC address entries across interfaces.The current code checks for the MAC address only on the current interface (
interface.mac_addresses.filter()), then creates a newMACAddressif not found locally. Since NetBox'sMACAddressmodel has no unique constraint on themac_addressfield, the same MAC string can be created as a separate database record if it exists on a different interface. UseMACAddress.objects.get_or_create()to check globally and reuse existing records.Proposed fix
def handle_mac_address(self, interface, ifPhysAddress): """Assign or create the MAC address for the given interface.""" if ifPhysAddress: - existing_mac = interface.mac_addresses.filter(mac_address=ifPhysAddress).first() - if existing_mac: - mac_obj = existing_mac - else: - mac_obj = MACAddress.objects.create(mac_address=ifPhysAddress) + mac_obj, _ = MACAddress.objects.get_or_create(mac_address=ifPhysAddress) interface.mac_addresses.add(mac_obj) interface.primary_mac_address = mac_obj🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 160 - 170, The handle_mac_address function currently queries only interface.mac_addresses and may create duplicate MACAddress rows; change it to use MACAddress.objects.get_or_create(mac_address=ifPhysAddress) to fetch or create a global MACAddress, then add that returned mac_obj to interface.mac_addresses and set interface.primary_mac_address to it (keep the existing guard for ifPhysAddress and ensure you use the tuple returned by get_or_create to obtain the MACAddress instance).
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/scripts/setup.sh:
- Line 124: The mkdir invocation `mkdir -p -m 755 /etc/apt/keyrings` only
applies the mode to the leaf directory; change it so the mode is reliably set
regardless of existing parents by either (a) creating with `mkdir -p
/etc/apt/keyrings` then running `chmod 755 /etc/apt/keyrings`, or (b) replace
with `install -d -m 755 /etc/apt/keyrings`; update the line containing `mkdir -p
-m 755 /etc/apt/keyrings` accordingly.
- Around line 281-295: Currently the script silently rewrites SSH Git remotes to
HTTPS; update it to respect an opt-out env var (e.g., PLUGIN_PRESERVE_GIT_SSH)
and emit a more prominent notice before changing remotes: check
PLUGIN_PRESERVE_GIT_SSH (or similar) and skip the conversion if set, and when
performing the change (the block that reads CURRENT_REMOTE, computes HTTPS_URL
and runs git remote set-url origin) print a clear, visible warning to the user
and instructions on how to opt out (and log the original and new URL); keep the
existing behavior when the env var is not set.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 796-807: The current error handler alerts raw error.message (e.g.
"HTTP 500: Internal Server Error") which may confuse users; change the alert to
show a friendly fallback (e.g. "An error occurred while loading results. Please
try again.") while retaining console.error(error) for debugging. Concretely, in
the catch block that references error, errorMsg, filterModalManager.hide(), and
currentAbortController = null, compute a userMessage = 'An error occurred while
loading results. Please try again.' and call alert(userMessage) instead of
alert(errorMsg), but keep console.error('Error fetching filtered results:',
error) so the raw error is still logged.
In `@netbox_librenms_plugin/tables/device_status.py`:
- Line 262: Remove the redundant local imports of reverse inside the helper
functions render_netbox_cluster, render_netbox_role, and render_netbox_rack and
rely on the module-level import already present; edit each function to delete
the line "from django.urls import reverse" so there are no duplicate imports
while preserving existing uses of reverse in those functions.
- Around line 94-108: The constructor (__init__) eagerly loads all Cluster and
DeviceRole records into self._cached_clusters and self._cached_roles which can
blow memory; change this to lazy loading by replacing the immediate list() calls
with deferred access (e.g., use a cached_property or getter) so the DB is only
hit when _cached_clusters/_cached_roles are first accessed, or restrict the
query to only the items needed for the current table (e.g., filter by IDs
present in the table data or use values_list to fetch only names/ids). Update
references to these attributes to call the lazy getter (or property) and keep
the sorting call to _sort_data() intact so behavior is unchanged.
- Around line 138-144: The code currently mutates the internal django-tables2
structure by calling self.data.data.sort(...) inside DeviceImportTable; instead,
remove that mutation and sort the raw list of dicts in the view before
constructing DeviceImportTable. Locate where the view builds the table (passes
data into DeviceImportTable) and, when the dataset is a list, call sorted(data,
key=sort_key, reverse=reverse) and pass that sorted list as the data argument to
DeviceImportTable; also delete or revert the try/except block that references
self.data.data and self.data so the table class no longer touches internal
attributes.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Around line 213-216: The template loop over
interface_sync.virtual_chassis_members currently outputs a stray text node "{{
member.name }}" before the option, causing unstructured names to appear; remove
that standalone "{{ member.name }}" (or move it inside the <option> if intended)
so only the <option value="{{ member.id }}">{{ member.name }}</option>
remains—adjust the loop around interface_sync.virtual_chassis_members and
references to member.name and member.id to eliminate the duplicate/bare output.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 166-175: The opening <form> tags are missing their closing ">" so
the {% csrf_token %} and <input> elements are being treated as attributes; fix
each malformed form by adding the closing ">" immediately after the hx-swap
attribute (i.e. end the <form ... hx-swap="none">) before the csrf_token and
inputs. Apply this same fix for each occurrence of the device_conflict_action
form (the forms posting to {% url
'plugins:netbox_librenms_plugin:device_conflict_action'
device_id=libre_device.device_id %} that contain hidden inputs named
"existing_device_id" and "action" with values like "sync_device_type", etc.).
- Around line 547-551: The link construction using {{ existing_device_url
}}librenms-sync/ is fragile; update the template branch that checks
validation.existing_match_type == 'librenms_id' to generate the full-sync URL
via Django's URL resolver instead of string-concatenation—use the device's
identifier (e.g., device.pk or validation.existing_device.pk) with a {% url %}
call for the named view (the view name used for the librenms sync page in this
plugin) so the link remains correct regardless of trailing slashes or URL prefix
changes.
- Line 23: The header close button currently uses Bootstrap dismissal
(data-bs-dismiss="modal") which conflicts with the HTMX modal wrapper pattern;
remove the data-bs-dismiss attribute and instead add the attribute your
librenms_import.html modal JS listens for (targeting the htmx-modal-content
wrapper) e.g. set a data attribute that points to "htmx-modal-content" (same
pattern used by the footer close button) so the JS can toggle/close the wrapper;
also update the footer close button (the similar instance flagged earlier) to
use the same HTMX-targeting attribute and ensure no data-bs-toggle or duplicate
modal IDs are introduced.
In `@netbox_librenms_plugin/utils.py`:
- Around line 192-197: The get_interface_name_field function currently writes
the interface_name_field to user prefs on any GET parameter presence, causing
unnecessary DB writes; change the logic to persist only on POST requests or only
when the new value differs from the stored preference: inside
get_interface_name_field, check request.method == "POST" before calling
_save_user_pref, or fetch the existing preference
(plugins.netbox_librenms_plugin.interface_name_field) and compare it to
param_val and call _save_user_pref only if they differ; reference
get_interface_name_field and _save_user_pref when making the change.
In `@netbox_librenms_plugin/views/settings_views.py`:
- Around line 132-183: The response HTML is currently embedding unescaped
external data (LibreNMS API fields: version, database, php_version; API
error_msg; and exception strings from except blocks) which creates an XSS risk;
update the view in settings_views.py to import django.utils.html.escape and wrap
all externally sourced values (version, database, php_version, error_msg, and
str(e) in the ValueError and general Exception handlers) with escape() before
interpolating into the HttpResponse f-strings (keep the existing success/failure
HTML structure and LibreNMSAPI/test_connection usage).
In `@netbox_librenms_plugin/views/status_check.py`:
- Around line 52-58: The loop in get_queryset (and the similar VM method) calls
self.librenms_api.get_librenms_id(device) per object causing N+1 API calls;
change this to a batched lookup by adding a bulk method on librenms_api (e.g.,
get_librenms_ids_for_devices or get_librenms_id_map) that accepts a list of
devices or IDs and returns a mapping of device.pk -> librenms_id, then replace
the per-device calls in get_queryset and the VM variant to call that bulk method
once and populate device_status_map[device.pk] = bool(mapping.get(device.pk)).
Ensure you update references to librenms_api.get_librenms_id to use the new bulk
method name and handle missing keys safely.
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 44-53: The create_cable function currently swallows exceptions and
returns None, so change create_cable (in
netbox_librenms_plugin.views.sync.cables) to return an explicit success flag
(True on successful Cable.objects.create, False on exception) or re-raise the
exception; keep the messages.error call but return False in the except block.
Then update handle_cable_creation to check the return value from create_cable
and only set {"status": "valid"} when create_cable returns True (otherwise set
an error/failed status and include the error context), ensuring callers rely on
the boolean instead of assuming success.
- Around line 112-121: process_interface_sync is wrapped in transaction.atomic
but create_cable swallows exceptions internally, preventing rollbacks; either
remove the internal try/except in create_cable (so exceptions from
Cable.objects.create propagate and atomic can rollback) or, if partial success
is desired, remove transaction.atomic from process_interface_sync and update
process_single_interface/create_cable to return explicit failure statuses (e.g.,
"invalid" or "duplicate") for any caught errors; locate create_cable and
process_interface_sync/process_single_interface to implement the chosen approach
and ensure results[result["status"]] correctly reflects failures when exceptions
occur.
- Around line 23-35: The get_selected_interfaces function is collecting a
device_id into each interface dict that is never used downstream; update
get_selected_interfaces to only append the interface name (e.g., {"interface":
interface} or even just the string if callers accept it) and remove the
device_id key/assignment and the
request.POST.get(f"device_selection_{interface}") lookup; ensure callers
(process_single_interface and handle_cable_creation) continue to access
interface["interface"] (or adapt them if you switch to a plain string) and that
link_data remains the source of device IDs for cable creation.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 350-358: AssignVCSerialView currently overwrites member.serial
before validation/save, so on failure the in-memory Member is left with the new
value; capture the original value (e.g., old_serial = member.serial) before
assigning serial, then in the except block restore member.serial = old_serial
(and optionally call member.full_clean()/member.save() only after successful
assignment) so the object state matches the persisted state; reference
AssignVCSerialView, member.serial, member.full_clean, and member.save when
making this change.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 237-250: Replace the two separate permission checks in
DeleteNetBoxInterfacesView.post with the single helper: after setting
self.required_object_permissions (as currently done), remove the calls to
require_write_permission_json() and require_object_permissions_json("POST") and
call require_all_permissions_json("POST") instead; this uses
NetBoxObjectPermissionMixin.require_all_permissions_json to perform the combined
check consistently with SyncInterfacesView.post.
- Around line 297-302: The current broad except in the loop (after catching
Interface.DoesNotExist and VMInterface.DoesNotExist) swallows all exceptions;
narrow this by removing the bare `except Exception` and explicitly handling
expected runtime errors (e.g., ValueError/KeyError) while re-raising or allowing
Django DB errors to propagate: catch and append messages for non-fatal
exceptions only, and explicitly `raise` for IntegrityError,
TransactionManagementError, DatabaseError (or re-raise any exception subclass
from django.db) so they break the transaction; reference the same block where
Interface.DoesNotExist and VMInterface.DoesNotExist are handled and the `errors`
list is appended to, and ensure any defensive logging preserves the original
exception when re-raising.
- Around line 160-170: The handle_mac_address function currently queries only
interface.mac_addresses and may create duplicate MACAddress rows; change it to
use MACAddress.objects.get_or_create(mac_address=ifPhysAddress) to fetch or
create a global MACAddress, then add that returned mac_obj to
interface.mac_addresses and set interface.primary_mac_address to it (keep the
existing guard for ifPhysAddress and ensure you use the tuple returned by
get_or_create to obtain the MACAddress instance).
| {% endif %} | ||
| Import Validation: {{ libre_device.sysName|default:libre_device.hostname }} | ||
| </h5> | ||
| <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button> |
There was a problem hiding this comment.
Header close button uses data-bs-dismiss — should use the HTMX modal mechanism.
The coding guidelines state modals use the HTMX wrapper toggled by JS in librenms_import.html, not Bootstrap's dismiss. This data-bs-dismiss="modal" may not close the modal if Bootstrap JS isn't loaded, and conflicts with the HTMX-driven modal pattern. The same issue was previously flagged for the footer close button (line 555).
As per coding guidelines: "Modal buttons should target the htmx-modal-content element. JavaScript in librenms_import.html toggles the wrapper; do not reintroduce data-bs-toggle or duplicate modal IDs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`
at line 23, The header close button currently uses Bootstrap dismissal
(data-bs-dismiss="modal") which conflicts with the HTMX modal wrapper pattern;
remove the data-bs-dismiss attribute and instead add the attribute your
librenms_import.html modal JS listens for (targeting the htmx-modal-content
wrapper) e.g. set a data attribute that points to "htmx-modal-content" (same
pattern used by the footer close button) so the JS can toggle/close the wrapper;
also update the footer close button (the similar instance flagged earlier) to
use the same HTMX-targeting attribute and ensure no data-bs-toggle or duplicate
modal IDs are introduced.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/views/sync/device_fields.py (1)
280-308:⚠️ Potential issue | 🟡 MinorOrphaned Platform if device assignment fails after platform creation.
If
Platform.objects.create()succeeds (line 281) but the subsequentdevice.save()fails (line 297), the newly created Platform remains in the database without being assigned. Consider wrapping both operations in a singletransaction.atomic()block so the platform creation is rolled back if device assignment fails.Proposed fix
+from django.db import transaction + ... + with transaction.atomic(): - try: - platform = Platform.objects.create( - name=platform_name, - manufacturer=manufacturer, - ) - except IntegrityError: - messages.error( - request, - f"Platform '{platform_name}' could not be created (slug collision). Try a different name.", - ) - return redirect("plugins:netbox_librenms_plugin:device_librenms_sync", pk=pk) - - old_platform = device.platform - device.platform = platform - try: - device.full_clean() - device.save() - except (ValidationError, IntegrityError) as e: - device.platform = old_platform - error_msg = e.message_dict if hasattr(e, "message_dict") else str(e) - messages.error(request, f"Failed to assign platform '{platform}': {error_msg}") - return redirect("plugins:netbox_librenms_plugin:device_librenms_sync", pk=pk) + try: + platform = Platform.objects.create( + name=platform_name, + manufacturer=manufacturer, + ) + except IntegrityError: + messages.error( + request, + f"Platform '{platform_name}' could not be created (slug collision). Try a different name.", + ) + return redirect("plugins:netbox_librenms_plugin:device_librenms_sync", pk=pk) + + old_platform = device.platform + device.platform = platform + try: + device.full_clean() + device.save() + except (ValidationError, IntegrityError) as e: + device.platform = old_platform + error_msg = e.message_dict if hasattr(e, "message_dict") else str(e) + messages.error(request, f"Failed to assign platform '{platform}': {error_msg}") + raise # triggers rollback of both platform creation and device saveNote: Using
raiseinside theatomic()block ensures the transaction rolls back, but you'll need to catch the re-raised exception at the outer level to return the redirect. An alternative is to use a savepoint or to explicitly delete the platform in the except block.🤖 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 280 - 308, The Platform created via Platform.objects.create(...) can be left orphaned if device.full_clean()/device.save() fails; wrap the create + assignment + device.full_clean()/device.save() inside a single transaction.atomic() so the Platform creation is rolled back on exceptions (IntegrityError/ValidationError), or alternatively explicitly delete the created platform in the except branch before returning; update the block that calls Platform.objects.create, assigns device.platform, calls device.full_clean and device.save to use transaction.atomic() (or delete the platform on failure) and ensure you still surface the error via messages.error and then return the redirect.netbox_librenms_plugin/views/sync/cables.py (1)
119-128: 🧹 Nitpick | 🔵 Trivial
transaction.atomic()wraps the loop but individual failures are silently absorbed.
create_cablecatches all exceptions and returnsFalse, meaning individual failures won't roll back previously created cables within the sameatomic()block. If partial creation is the intended behavior, thetransaction.atomic()wrapper is somewhat misleading — it only protects against uncaught exceptions. This is acceptable if partial cable creation is desired, but worth documenting the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 119 - 128, The outer transaction.atomic() in process_interface_sync gives a misleading guarantee because create_cable swallows exceptions and returns False, so failures don’t roll back prior work; either make each interface operation its own atomic block (move transaction.atomic() inside the loop or wrap the create_cable call in its own atomic() within process_single_interface) so individual failures roll back only that interface, or if partial-success is desired, remove the outer atomic() and add a clear comment documenting that behavior; update references to process_interface_sync, process_single_interface, and create_cable accordingly.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@docs/usage_tips/permissions.md`:
- Around line 20-30: Update the ordered list numbering to use a consistent "1."
prefix to satisfy MD029: change the "2. **Tier 2: Object permission**" list item
to "1. **Tier 2: Object permission**" (the item that mentions `dcim.add_device`)
so both top-level entries ("**Tier 1: Plugin permission**" and "**Tier 2: Object
permission**") use the same "1." prefix; ensure any other top-level ordered
items follow the same pattern.
In `@netbox_librenms_plugin/api/views.py`:
- Around line 21-33: Update the class docstring for LibreNMSPluginPermission to
show the full permission strings instead of the short names: replace the lines
that say "view_librenmssettings" and "change_librenmssettings" with
"netbox_librenms_plugin.view_librenmssettings" and
"netbox_librenms_plugin.change_librenmssettings" to match the constants
PERM_VIEW_PLUGIN and PERM_CHANGE_PLUGIN used in has_permission (and mention the
GET/SAFE_METHODS vs other requests behavior for clarity).
In `@netbox_librenms_plugin/forms.py`:
- Around line 50-79: The _get_librenms_poller_group_choices function currently
calls LibreNMSAPI().get_poller_groups on every form instantiation and swallows
all errors; change it to first try reading cached choices from django.core.cache
(use a clear cache key like "librenms_poller_group_choices" and a sensible
timeout similar to _populate_librenms_locations), only call
LibreNMSAPI.get_poller_groups when cache miss, store the constructed choices
list in cache, and replace the bare except with logging the exception (e.g.,
logger.exception or logger.error(..., exc_info=True)) so API/network errors are
recorded while still falling back to the default choice.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 338-349: The NetBox column is showing libre_device.ip with a
success icon when validation.existing_device exists but has no primary_ip;
update the template netbox_librenms_plugin/htmx/device_validation_details.html
so the NetBox cell only displays validation.existing_device.primary_ip (and the
success icon) when validation.existing_device.primary_ip is present, otherwise
render the "No primary IP" / "Not set" text; remove the elif branch that inserts
{{ libre_device.ip }} and the <i class="mdi mdi-check-circle"> icon in that
NetBox cell so libre_device.ip only appears in the LibreNMS column (keep the
mdi-check-circle only when validation.existing_device.primary_ip is present).
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 2121-2127: The test is patching dcim.models.Platform but the code
path now calls netbox_librenms_plugin.utils.find_matching_platform via
_build_sync_info; update the test to patch
netbox_librenms_plugin.utils.find_matching_platform (instead of patching
dcim.models.Platform) and have that mock return the platform (or {"matched":
True, "device_type": device_type} as appropriate); keep the existing patch of
netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type if still
needed, and remove/replace the mock_platform_cls.objects.get setup so
find_matching_platform provides the platform directly.
In `@netbox_librenms_plugin/utils.py`:
- Around line 145-158: The helpers are defined as module-private
(_get_user_pref, _save_user_pref) but are imported elsewhere; make them public
by renaming to get_user_pref and save_user_pref (or add public wrapper functions
that call the underscored versions) so cross-module imports are correct; update
all references/imports (e.g., in views/imports/list.py) to the new names and run
tests/lint to ensure no remaining underscore usages.
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 82-88: The StopIteration handler in process_single_interface
returns {"status": "invalid"} without the interface identifier, causing empty
entries in flash messages; modify process_single_interface so the except block
returns {"status": "invalid", "interface": interface["interface"]} (or
interface.get("interface","") to be defensive) so process_interface_sync can
correctly group invalid interfaces by name and avoid empty elements in the
flashed message.
- Around line 119-128: The outer transaction.atomic() in process_interface_sync
gives a misleading guarantee because create_cable swallows exceptions and
returns False, so failures don’t roll back prior work; either make each
interface operation its own atomic block (move transaction.atomic() inside the
loop or wrap the create_cable call in its own atomic() within
process_single_interface) so individual failures roll back only that interface,
or if partial-success is desired, remove the outer atomic() and add a clear
comment documenting that behavior; update references to process_interface_sync,
process_single_interface, and create_cable accordingly.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 12-56: The four single-field sync views (UpdateDeviceNameView,
UpdateDeviceSerialView, UpdateDeviceTypeView, UpdateDevicePlatformView)
duplicate the same preamble (permission check, get_object_or_404,
get_librenms_id, get_device_info, field extraction and early-returns); extract
that logic into a shared helper (e.g., a new method get_device_and_info(self,
request, pk) on a common base/mixin such as LibreNMSAPIMixin or a new
SyncDeviceMixin) that returns (device, device_info) or performs the
redirects/messages on failure, and then replace the repeated preamble in each
view with a single call to get_device_and_info and continue only if it returns
valid results (update references to self.librenms_id if needed).
- Around line 280-308: The Platform created via Platform.objects.create(...) can
be left orphaned if device.full_clean()/device.save() fails; wrap the create +
assignment + device.full_clean()/device.save() inside a single
transaction.atomic() so the Platform creation is rolled back on exceptions
(IntegrityError/ValidationError), or alternatively explicitly delete the created
platform in the except branch before returning; update the block that calls
Platform.objects.create, assigns device.platform, calls device.full_clean and
device.save to use transaction.atomic() (or delete the platform on failure) and
ensure you still surface the error via messages.error and then return the
redirect.
In `@netbox_librenms_plugin/views/sync/devices.py`:
- Around line 14-22: get_form_class duplicates the SNMP version extraction later
in post; compute and store the resolved version once (e.g. set
self._snmp_version inside get_form_class using the existing logic that reads
self.request.POST.get("snmp_version") with fallbacks to "v1v2-snmp_version" and
"v3-snmp_version") and then return AddToLIbreSNMPV1V2 or AddToLIbreSNMPV3 as
before; update post to read from self._snmp_version instead of re-extracting,
and ensure a safe fallback (re-run the same resolution or default) if
self._snmp_version is not set to avoid regressions.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
.devcontainer/README.md.devcontainer/scripts/setup.sh.github/workflows/test.yamldocs/usage_tips/permissions.mdnetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/settings_views.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.py
🧰 Additional context used
📓 Path-based instructions (6)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Modals use Tabler (Bootstrap-like) but withoutbootstrap.Modalhelpers.
Modal buttons should target thehtmx-modal-contentelement. JavaScript inlibrenms_import.htmltoggles the wrapper; do not reintroducedata-bs-toggleor duplicate modal IDs.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up with TomSelect decorators.
Styling assumes Tabler defaults. Do not addtable-responsivewrappers as they were deliberately removed to prevent dropdown clipping.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Templates should live in
templates/netbox_librenms_plugin/. Reusable template includes should be placed underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments should live in
templates/netbox_librenms_plugin/htmx/. Keep server responses and HTMX targets in sync.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuse thelibrenms_api.pyLibreNMS client instead of making newrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching
Always callLibreNMSAPI.get_librenms_idto retrieve the device/VM LibreNMS mapping via thelibrenms_idcustom field instead of touching the field directly
Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functionsfind_matching_site,match_librenms_hardware_to_device_type, andfind_matching_platformfromutils.py
Files:
netbox_librenms_plugin/views/settings_views.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/views/sync/devices.py
netbox_librenms_plugin/templates/netbox_librenms_plugin/*sync*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Sync pages should extend
librenms_sync_base.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: Use Job UUID (job.job_id) for RQ API endpoints:/api/core/background-tasks/{uuid}/
Use Job PK (job.pk) for database endpoints and result loading
Checkrq_job.is_stoppedorrq_job.is_failedflags in Redis for cancellation detection, not database status
Call/api/core/background-tasks/{uuid}/stop/to stop RQ job in the job cancellation flow
Call plugin's sync endpoint/api/plugins/librenms_plugin/jobs/{pk}/sync-status/to update database after stopping RQ job
Poll/api/core/background-tasks/{uuid}/for real-time RQ status instead of polling the database
api/views.py::sync_job_status()must sync database Job status with RQ job status because NetBox worker doesn't always update DB when jobs stop before processing starts
Files:
netbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.py
🧠 Learnings (31)
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Use devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) to manage NetBox + plugin reloading during development
Applied to files:
.github/workflows/test.yaml
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/netbox_librenms_plugin/*sync*.html : Sync pages should extend `librenms_sync_base.html`.
Applied to files:
docs/usage_tips/permissions.mdnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/devices.py
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modal buttons should target the `htmx-modal-content` element. JavaScript in `librenms_import.html` toggles the wrapper; do not reintroduce `data-bs-toggle` or duplicate modal IDs.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Avoid `outerHTML` swaps in HTMX; use OOB or targeted `innerHTML` swaps to keep table layout intact.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : HTMX 2.x is the primary async layer. Table row updates should return `<tr hx-swap-oob="true">`.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/devices.py
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/**/*.html : HTMX fragments should live in `templates/netbox_librenms_plugin/htmx/`. Keep server responses and HTMX targets in sync.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to {jobs.py,views/imports/**/*.py} : Background job files and import views follow conventions documented in `.github/instructions/background-jobs.instructions.md`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Keep `<select class="device-role-select">` markup stable to preserve JavaScript hook-up with TomSelect decorators.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Styling assumes Tabler defaults. Do not add `table-responsive` wrappers as they were deliberately removed to prevent dropdown clipping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_background_jobs.py : Test view decision logic by setting `view._filter_form_data = {...}` directly, not via HTTP requests
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to templates/**/*.html : Reuse template includes under `templates/netbox_librenms_plugin/inc/` and ensure sync pages extend `librenms_sync_base.html`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/devices.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to tables/**/*.py : Prefer updating the table renderer in `tables/*.py` rather than templates when changing row actions, since tables emit HTMX-enabled columns and buttons
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Tables drive most UIs via `tables/*.py` renderers that emit HTMX-enabled columns and buttons. Prefer updating the table renderer in Python rather than templates when changing row actions.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to **/*.py : Always call `LibreNMSAPI.get_librenms_id` to retrieve the device/VM LibreNMS mapping via the `librenms_id` custom field instead of touching the field directly
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to {navigation.py,urls.py,api/**} : Respect NetBox plugin APIs in `navigation.py`, `urls.py`, and `api/` directories
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/devices.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Follow sync pipeline flow: fetch LibreNMS data via `librenms_api.py`, cache it using `CacheMixin`, build comparison tables in `tables/`, and render HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/devices.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
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
Applied to files:
netbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to **/*.py : Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functions `find_matching_site`, `match_librenms_hardware_to_device_type`, and `find_matching_platform` from `utils.py`
Applied to files:
netbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/jobs.py : Background jobs must use NetBox's `JobRunner` base class (`netbox.jobs.JobRunner`) for long-running operations like device filtering with VC detection
Applied to files:
netbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/imports/list.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/**/*.py : Test coverage mapping: `librenms_api.py` → `test_librenms_api.py`, `import_utils.py`/`import_validation_helpers.py`/`utils.py` → `test_import_utils.py`/`test_import_validation_helpers.py`/`test_utils.py`, `jobs.py`/`views/imports/list.py` → `test_background_jobs.py`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_import_utils.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_background_jobs.py : Patch `cache` where imported: `netbox_librenms_plugin.views.imports.list.cache`, not `django.core.cache.cache`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/imports/list.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : `api/views.py::sync_job_status()` must sync database Job status with RQ job status because NetBox worker doesn't always update DB when jobs stop before processing starts
Applied to files:
netbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/imports/list.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : Call plugin's sync endpoint `/api/plugins/librenms_plugin/jobs/{pk}/sync-status/` to update database after stopping RQ job
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/jobs.py : Jobs must run via Redis Queue (RQ) in Redis, separate from the database Job model, and real-time status must be checked via RQ, not the database
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : Use Job UUID (`job.job_id`) for RQ API endpoints: `/api/core/background-tasks/{uuid}/`
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : Poll `/api/core/background-tasks/{uuid}/` for real-time RQ status instead of polling the database
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to views/**/*.py : Views should follow layered structure: extend the closest base class from `views/base/` and compose mixins like `LibreNMSAPIMixin` and `CacheMixin`
Applied to files:
netbox_librenms_plugin/views/sync/cables.py
🧬 Code graph analysis (7)
netbox_librenms_plugin/views/settings_views.py (2)
netbox_librenms_plugin/views/mixins.py (2)
LibreNMSPermissionMixin(28-82)require_write_permission(43-65)netbox_librenms_plugin/utils.py (1)
_save_user_pref(152-158)
netbox_librenms_plugin/tables/interfaces.py (3)
netbox_librenms_plugin/tables/VM_status.py (1)
Meta(36-56)netbox_librenms_plugin/tables/device_status.py (2)
Meta(55-82)Meta(672-712)netbox_librenms_plugin/tables/ipaddresses.py (1)
Meta(18-36)
netbox_librenms_plugin/tests/test_librenms_api.py (1)
netbox_librenms_plugin/librenms_api.py (1)
add_device(363-427)
netbox_librenms_plugin/tests/test_import_utils.py (2)
netbox_librenms_plugin/import_utils.py (1)
validate_device_for_import(634-1122)netbox_librenms_plugin/views/imports/actions.py (10)
DeviceConflictActionView(819-1002)post(213-362)post(389-665)post(783-790)post(796-803)post(809-816)post(822-1002)post(1014-1030)DeviceValidationDetailsView(694-777)_build_sync_info(724-777)
netbox_librenms_plugin/api/views.py (2)
netbox_librenms_plugin/models.py (1)
InterfaceTypeMapping(51-76)netbox_librenms_plugin/api/serializers.py (1)
InterfaceTypeMappingSerializer(6-13)
netbox_librenms_plugin/views/sync/cables.py (1)
netbox_librenms_plugin/views/mixins.py (5)
CacheMixin(285-306)LibreNMSPermissionMixin(28-82)NetBoxObjectPermissionMixin(85-193)get_cache_key(290-299)require_all_permissions(167-179)
netbox_librenms_plugin/forms.py (2)
netbox_librenms_plugin/views/mixins.py (1)
librenms_api(217-231)netbox_librenms_plugin/librenms_api.py (2)
LibreNMSAPI(15-855)get_poller_groups(678-709)
🪛 LanguageTool
docs/usage_tips/permissions.md
[style] ~152-~152: Consider an alternative verb to strengthen your wording.
Context: ...ound job APIs should not appear. If you see these errors, it usually means a direct...
(IF_YOU_HAVE_THIS_PROBLEM)
🪛 markdownlint-cli2 (0.20.0)
docs/usage_tips/permissions.md
[warning] 29-29: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🔇 Additional comments (36)
.github/workflows/test.yaml (1)
67-67: LGTM — quoting prevents word splitting (SC2046).netbox_librenms_plugin/tables/interfaces.py (1)
24-25: Docstring additions look good.Consistent with the pattern used across sibling table files (
VM_status.py,cables.py,ipaddresses.py).Also applies to: 43-43, 323-323, 363-363, 369-370, 394-395
.devcontainer/README.md (1)
162-162: Grammar fix applied correctly.The addition of the definite article "the" improves readability and addresses the previous review feedback. The sentence now reads naturally.
.devcontainer/scripts/setup.sh (1)
74-76: Good addition for isolated virtualenv compatibility.Setting
global.certviapip configensures tools likepre-committhat create their own virtualenvs will use the system CA bundle rather than the bundledcertifi. The|| trueguard is appropriate.netbox_librenms_plugin/api/views.py (2)
44-45: Good use of DRF decorators for the function-based view.Replacing manual HTTP method checks with
@api_view(["POST"])and@permission_classesis the idiomatic DRF approach. This gives you proper method validation, content negotiation, and consistent permission enforcement.
35-41: Remove this comment—the API permission implementation is correct per documented plugin architecture.The copilot instructions explicitly specify: "API endpoints use
LibreNMSPluginPermissionclass inapi/views.py(GET=view, others=change)." TheInterfaceTypeMappingViewSetcorrectly implements this documented pattern. The plugin uses a two-tier permission system (viaview_librenmssettingsandchange_librenmssettings), not NetBox token constraints, so overridingpermission_classeswithLibreNMSPluginPermissionis the intended design.Likely an incorrect or invalid review comment.
netbox_librenms_plugin/views/sync/cables.py (1)
10-21: Permission setup and class structure look solid.Clean MRO with
LibreNMSPermissionMixin,NetBoxObjectPermissionMixin,CacheMixin, andView. Therequired_object_permissionscorrectly requires bothaddandchangeonCablefor POST, andrequire_all_permissionsinpost()gates both plugin-level and object-level permissions.netbox_librenms_plugin/views/sync/device_fields.py (2)
12-56: UpdateDeviceNameView: clean implementation with proper error handling.Good pattern: permission check → fetch → validate → update with
full_clean()/save()→ revert on failure. Theold_namecapture and revert is correct, and the error message properly surfaces validation details.
333-369: AssignVCSerialView:whileloop driven by POST keys is fragile but functional.The
while f"serial_{counter}" in request.POSTpattern (line 334) works correctly but couples tightly to the template's naming convention. If a gap appears in numbering (e.g.,serial_1,serial_3withoutserial_2), the loop stops early and silently skips remaining members. This is acceptable if the template guarantees contiguous numbering, but worth noting.The per-member error handling (lines 352-360) with
old_serialcapture is well done.netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (5)
28-35: Good: pre-computedexisting_device_urlwith VM-aware URL resolution.This addresses the prior concern about hardcoded device URLs for VMs. The URL is computed once and reused throughout the template, keeping links consistent.
104-114: Forms correctly usehx-swap="none"— addresses prior outerHTML guideline concern.All inline sync forms now use
hx-swap="none"instead of the previously flaggedouterHTML, consistent with the coding guideline to avoidouterHTMLswaps. The server-side response (viaHX-Trigger) handles row updates out-of-band.
38-79: Well-structured two-column layout with clean conditional rendering.The LibreNMS Status card (left) and Device Information table (right) are cleanly separated. The conditional rendering for VM vs. Device contexts (Site/DeviceType/Serial hidden for VMs, Cluster shown instead of Rack) is thorough and consistent.
Also applies to: 81-354
357-515: Status & Actions: comprehensive match-type handling with appropriate blocking.The serial conflict blocking at lines 398-403 ("Import blocked") correctly prevents linking when a serial conflict exists. The force-toggle pattern for type mismatches (lines 412-419, 460-467) provides a deliberate friction mechanism. The cascading match types (librenms_id → hostname → serial → primary_ip → fallback) cover the expected scenarios well.
547-552: Full Sync Page link uses{% url %}resolver — solid fix from prior feedback.The link now uses
{% url 'plugins:netbox_librenms_plugin:device_librenms_sync' pk=validation.existing_device.pk %}instead of string concatenation, which is robust against URL prefix changes.netbox_librenms_plugin/views/imports/actions.py (3)
210-227: Permission gate before bulk import confirmation is solid.This keeps the modal flow consistent with write-access requirements.
365-387: Background-job gating for non‑superusers looks correct.Clear fallback path and explicit note about the superuser-only API.
712-777: Sync-info builder correctly reuses matching helpers.Good consistency with the exact-match policy and safe device_type checks.
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
159-165: Nice empty-state UX.Clear guidance to refresh interfaces and consistent styling.
netbox_librenms_plugin/tests/test_librenms_api.py (1)
592-682: SNMPv1/v3 payload assertions are comprehensive.Great coverage for required fields and community exclusion for v3.
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html (1)
201-219: UI gating and preference persistence look clean.Disabled background-job UI for non‑superusers and CSRF‑aware preference saves are well‑handled.
Also applies to: 350-359, 401-431, 436-466
netbox_librenms_plugin/views/settings_views.py (1)
45-91: Permission gating + escaped connection details look great.Good defense-in-depth for POST actions and safer rendering of API data.
Also applies to: 140-155
netbox_librenms_plugin/forms.py (2)
93-102: LGTM — docstrings and shared helper usage.The Meta docstrings and wiring of
_get_librenms_server_choices/_get_librenms_poller_group_choicesin form__init__methods are clean and consistent.
273-330: LGTM — SNMPv1/v2c form rename and refactor.Removing
snmp_versionfrom the form (delegated to template toggle) and using the shared poller group helper is a clean approach. The__init__correctly calls the shared helper.netbox_librenms_plugin/utils.py (2)
161-176: LGTM — toggle persistence logic.Treating absent checkbox values as
Falseforhx-includeis correct. The implementation is clean and well-documented.
192-207: LGTM — write-on-GET issue addressed.The comparison against the existing preference before writing (line 197) prevents redundant DB writes, properly addressing the prior review feedback.
netbox_librenms_plugin/views/sync/devices.py (4)
24-29: LGTM — safer VM fallback.Using
get_object_or_404for theVirtualMachinelookup properly returns a 404 instead of an unhandled exception. Addresses the prior review feedback.
31-53: LGTM — permission check and form flow.The walrus-operator permission guard and the v1/v2c snmp_version injection into cleaned_data are clean. Error messages for invalid forms are properly surfaced via
messages.error.
55-101: LGTM —form_validhandles all SNMP versions with proper fallback.The
elsebranch (line 91–93) correctly catches unknown SNMP versions and redirects with an error message. The v1/v2c vs v3 payload construction is straightforward.
104-134: LGTM — UpdateDeviceLocationView with permission check.Write permission is properly enforced before the LibreNMS API call. Uses
get_librenms_idper coding guidelines.netbox_librenms_plugin/views/imports/list.py (3)
310-318: LGTM — user preference fallback now matches model defaults.
use_sysnamefalls back toTrue(matchingLibreNMSSettings.use_sysname_default) andstrip_domaintoFalse. This correctly addresses the prior review feedback about divergent fallback defaults.
54-63: LGTM — superuser gate for background jobs.The guard at line 61–62 correctly prevents non-superusers from using background jobs since the
/api/core/background-tasks/endpoint requires superuser access. Thecan_use_background_jobscontext flag (line 341) stays consistent with this logic.
345-361: LGTM — queryset and table methods.The docstrings clarify the delegation pattern.
get_tablecorrectly uses_import_datapopulated by_get_import_queryset.netbox_librenms_plugin/views/sync/interfaces.py (4)
16-37: LGTM — dynamic permission model.The
get_required_permissions_for_object_typepattern cleanly maps object types to the correct model permissions, andrequire_all_permissionsconsolidates both plugin-write and NetBox object permission checks into a single call.
121-125: LGTM — robust device ID handling.Catching
ValueErrorandTypeErroralongsideDoesNotExistproperly handles non-numeric POST input forselected_device_id. Addresses the prior review feedback.
147-158: LGTM — safe dict access for ifSpeed/ifType.Using
.get()instead of direct dict indexing preventsKeyErrorwhen LibreNMS port data is missing these keys. Addresses the prior review feedback.
225-246: LGTM — consolidated permission check in DeleteNetBoxInterfacesView.Using
require_all_permissions_json("POST")replaces the prior split into two separate calls, consistent withSyncInterfacesView. Addresses the prior review feedback.
| with ( | ||
| patch("dcim.models.Platform") as mock_platform_cls, | ||
| patch("netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type") as mock_hw_match, | ||
| ): | ||
| mock_platform_cls.objects.get.return_value = platform | ||
| mock_hw_match.return_value = {"matched": True, "device_type": device_type} | ||
|
|
There was a problem hiding this comment.
Patch find_matching_platform (current code path), not dcim.models.Platform.
_build_sync_info now calls find_matching_platform, so patch that helper to avoid hitting the real DB.
🛠️ Suggested fix
- with (
- patch("dcim.models.Platform") as mock_platform_cls,
- patch("netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type") as mock_hw_match,
- ):
- mock_platform_cls.objects.get.return_value = platform
+ with (
+ patch("netbox_librenms_plugin.utils.find_matching_platform") as mock_find_platform,
+ patch("netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type") as mock_hw_match,
+ ):
+ mock_find_platform.return_value = {"found": True, "platform": platform}
mock_hw_match.return_value = {"matched": True, "device_type": device_type}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tests/test_import_utils.py` around lines 2121 - 2127,
The test is patching dcim.models.Platform but the code path now calls
netbox_librenms_plugin.utils.find_matching_platform via _build_sync_info; update
the test to patch netbox_librenms_plugin.utils.find_matching_platform (instead
of patching dcim.models.Platform) and have that mock return the platform (or
{"matched": True, "device_type": device_type} as appropriate); keep the existing
patch of netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type if
still needed, and remove/replace the mock_platform_cls.objects.get setup so
find_matching_platform provides the platform directly.
| def _get_user_pref(request, path, default=None): | ||
| """Get a user preference value via request.user.config.""" | ||
| if hasattr(request, "user") and hasattr(request.user, "config"): | ||
| return request.user.config.get(path, default) | ||
| return default | ||
|
|
||
|
|
||
| def _save_user_pref(request, path, value): | ||
| """Save a user preference value via request.user.config.""" | ||
| if hasattr(request, "user") and hasattr(request.user, "config"): | ||
| try: | ||
| request.user.config.set(path, value, commit=True) | ||
| except (TypeError, ValueError): | ||
| pass |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Internal helpers imported across modules.
_get_user_pref is prefixed with _ (conventionally module-private) but is imported by views/imports/list.py. Consider dropping the underscore prefix for helpers intended for cross-module use, or consolidating access through a public wrapper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/utils.py` around lines 145 - 158, The helpers are
defined as module-private (_get_user_pref, _save_user_pref) but are imported
elsewhere; make them public by renaming to get_user_pref and save_user_pref (or
add public wrapper functions that call the underscored versions) so cross-module
imports are correct; update all references/imports (e.g., in
views/imports/list.py) to the new names and run tests/lint to ensure no
remaining underscore usages.
| def get_form_class(self): | ||
| """Return the appropriate SNMP form class based on the SNMP version.""" | ||
| snmp_version = self.request.POST.get("snmp_version") | ||
| if not snmp_version: | ||
| snmp_version = self.request.POST.get("v2-snmp_version") or self.request.POST.get("v3-snmp_version") | ||
| snmp_version = self.request.POST.get("v1v2-snmp_version") or self.request.POST.get("v3-snmp_version") | ||
|
|
||
| if snmp_version == "v2c": | ||
| return AddToLIbreSNMPV2 | ||
| if snmp_version in ("v1", "v2c"): | ||
| return AddToLIbreSNMPV1V2 | ||
| return AddToLIbreSNMPV3 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicated SNMP version extraction between get_form_class and post.
Lines 16–18 extract snmp_version to choose the form class, then lines 40–41 repeat the same extraction. Consider storing the resolved version as self._snmp_version in get_form_class (or a shared helper) to avoid the duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/devices.py` around lines 14 - 22,
get_form_class duplicates the SNMP version extraction later in post; compute and
store the resolved version once (e.g. set self._snmp_version inside
get_form_class using the existing logic that reads
self.request.POST.get("snmp_version") with fallbacks to "v1v2-snmp_version" and
"v3-snmp_version") and then return AddToLIbreSNMPV1V2 or AddToLIbreSNMPV3 as
before; update post to read from self._snmp_version instead of re-extracting,
and ensure a safe fallback (re-run the same resolution or default) if
self._snmp_version is not set to avoid regressions.
Security:
- Add LibreNMSPermissionMixin to DeviceConflictActionView with write
permission check
- Use SAFE_METHODS instead of hardcoded 'GET' in LibreNMSPluginPermission
- Escape LibreNMS API data in settings test_connection HTML responses (XSS)
Bug fixes:
- Close 4 <form> tags missing '>' after hx-swap attribute in validation
modal (csrf_token and inputs were parsed as tag attributes)
- Use {% url %} for Full Sync Page link instead of string concatenation
(was constructing wrong URL path)
- Remove stray {{ member.name }} text node before <option> in VC member
select modal
- Fix use_sysname fallback default to True (matches model default)
- Reset interface_name at top of delete loop to prevent stale names in
error messages
- Wrap ifAdminStatus in str() to prevent AttributeError on int values
- Use get_object_or_404 for VirtualMachine fallback in AddDeviceToLibreNMS
- Fix test status_code from 500 to 200 to match actual code path tested
- Restore member.serial on validation failure in AssignVCSerialView
- Return explicit bool from create_cable; handle_cable_creation checks
return value and reports failure status
- Include interface name in StopIteration result from
process_single_interface so flash messages show the affected interface
- Wrap Platform create + device assignment in transaction.atomic() in
CreateAndAssignPlatformView to prevent orphaned platforms on save failure
- Move transaction.atomic() to per-interface scope in cable sync so
individual failures roll back only that cable
Code quality:
- Extract _get_librenms_poller_group_choices() shared helper from two
identical form methods; add caching and exception logging
- Consolidate two permission checks into require_all_permissions_json
in DeleteNetBoxInterfacesView
- Save interface_name_field pref only when value differs from stored
- Add error handling to savePref JS (CSRF check, response/fetch errors)
- Use install -d -m 755 instead of mkdir -p -m 755 in setup.sh
- Add view.request mock for DeviceConflictActionView tests
- Update LibreNMSPluginPermission docstring with full permission strings
Docs:
- Fix grammar in README proxy section
- Add missing permission prefixes in permissions docs
- Fix heading spacing and ordered list numbering in permissions docs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
netbox_librenms_plugin/tests/test_librenms_api.py (1)
1179-1181: 🧹 Nitpick | 🔵 TrivialStale comment — "continuing in next part" is misleading.
All nine test classes are already present in this file. This trailing comment suggests the file is incomplete when it isn't.
🧹 Remove the stale comment
-# ==================================================================================== -# Test Class 5-9 continuing in next part due to length... -# Run `make unittest` to execute all tests -# ====================================================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_librenms_api.py` around lines 1179 - 1181, The trailing comment block (“# Test Class 5-9 continuing in next part due to length...”) is stale and misleading; remove that comment (the two lines shown in the diff) so the file no longer suggests missing tests—simply delete the commented lines to leave the file ending clean.netbox_librenms_plugin/views/sync/cables.py (2)
67-80: 🧹 Nitpick | 🔵 TrivialInconsistent
requestaccess pattern across methods.
validate_prerequisitesreadsself.request(set by Django'sView.setup()), while sibling methods likecreate_cableanddisplay_sync_resultsreceiverequestas an explicit parameter. Pick one convention for clarity — passing it explicitly is generally easier to test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 67 - 80, The method validate_prerequisites currently uses self.request while its sibling methods create_cable and display_sync_results accept request as an explicit parameter; update validate_prerequisites to accept request as the first argument (e.g., validate_prerequisites(self, request, cached_links, selected_interfaces)) and replace self.request usages with the passed request, then update all callers of validate_prerequisites (in the same class/view) to pass the request object; ensure signatures and tests are adjusted to use the explicit request convention consistently across create_cable, display_sync_results and validate_prerequisites.
23-35: 🧹 Nitpick | 🔵 TrivialRemove unused
device_idfrom interface dict.The
device_idextracted at line 32 is added to each selected interface dict but never accessed downstream — all methods only useinterface["interface"]for matching and reporting. Storing it is unnecessary and misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 23 - 35, In get_selected_interfaces, stop extracting and storing device_id since downstream code only uses the interface value; remove the device_id computation (the request.POST.get(f"device_selection_{interface}") or initial_device.id) and instead append only {"interface": interface} to selected_interfaces in the loop inside get_selected_interfaces so the returned dicts contain just the interface key.netbox_librenms_plugin/views/sync/interfaces.py (1)
160-169:⚠️ Potential issue | 🟠 MajorPrevent MAC uniqueness collisions during sync.
MACAddress.objects.create(...)can raise anIntegrityErrorwhen the MAC already exists. Useget_or_createto reuse existing records and keep sync resilient.🛠️ Proposed fix
- else: - mac_obj = MACAddress.objects.create(mac_address=ifPhysAddress) + else: + mac_obj, _ = MACAddress.objects.get_or_create(mac_address=ifPhysAddress)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 160 - 169, In handle_mac_address, avoid raising IntegrityError by replacing the direct create call with a get_or_create pattern: when ifPhysAddress is present, attempt to get_or_create the MACAddress (use MACAddress.objects.get_or_create(mac_address=ifPhysAddress)) and then add the returned instance to interface.mac_addresses; keep the prior check for existing_mac optional (you can simply use the get_or_create result) so duplicate MAC rows won’t cause failures during sync.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/scripts/setup.sh:
- Around line 54-66: Enable bash's nullglob around the cert-moving loop to avoid
iterating over the literal pattern when no files were created: locally set
nullglob before the for f in /usr/local/share/ca-certificates/proxy/cert-* loop
that renames fragments to .crt, run the mv commands as-is, then restore the
previous shell option after the loop; reference the for loop that processes
/usr/local/share/ca-certificates/proxy/cert-* and the CA_BUNDLE_SRC csplit step
when making the change.
In `@docs/usage_tips/permissions.md`:
- Around line 25-35: Replace the inconsistent "Netbox" capitalization with the
correct "NetBox" throughout the document—specifically change the occurrences
shown in the diff (the sentence beginning "allows performing actions that modify
Netbox or Librenms data", the heading/note referencing "Netbox object
permissions" and the line mentioning "The Plugin also enforces Netbox object
permissions") and the additional occurrence near the later mention (search for
the literal "Netbox" to find the remaining instance); keep the surrounding text
(e.g., "Tier 2: Object permission" and "dcim.add_device") unchanged.
In `@netbox_librenms_plugin/forms.py`:
- Around line 50-88: The cached_choices truthiness check in
_get_librenms_poller_group_choices can misinterpret an intentionally cached
empty list as a cache miss; update the conditional that reads cached_choices
(obtained via cache.get(cache_key)) to check for None explicitly (e.g.,
cached_choices is not None) so an empty list is returned from cache instead of
triggering another API call; keep the rest of the logic (including
cache.set(cache_key, choices, timeout=api.cache_timeout) and the default
("0","Default (0)") choice) unchanged.
In `@netbox_librenms_plugin/tests/test_librenms_api.py`:
- Around line 1179-1181: The trailing comment block (“# Test Class 5-9
continuing in next part due to length...”) is stale and misleading; remove that
comment (the two lines shown in the diff) so the file no longer suggests missing
tests—simply delete the commented lines to leave the file ending clean.
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 37-42: The method get_cached_links_data accepts an unused request
parameter; remove the request parameter from its signature and docstring, update
its body to use only obj (keep the cache.get(self.get_cache_key(obj, "links"))
logic and return behavior), and update every call site that currently passes a
request to instead pass only the obj when invoking get_cached_links_data so
signatures match.
- Around line 67-80: The method validate_prerequisites currently uses
self.request while its sibling methods create_cable and display_sync_results
accept request as an explicit parameter; update validate_prerequisites to accept
request as the first argument (e.g., validate_prerequisites(self, request,
cached_links, selected_interfaces)) and replace self.request usages with the
passed request, then update all callers of validate_prerequisites (in the same
class/view) to pass the request object; ensure signatures and tests are adjusted
to use the explicit request convention consistently across create_cable,
display_sync_results and validate_prerequisites.
- Around line 23-35: In get_selected_interfaces, stop extracting and storing
device_id since downstream code only uses the interface value; remove the
device_id computation (the request.POST.get(f"device_selection_{interface}") or
initial_device.id) and instead append only {"interface": interface} to
selected_interfaces in the loop inside get_selected_interfaces so the returned
dicts contain just the interface key.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 160-169: In handle_mac_address, avoid raising IntegrityError by
replacing the direct create call with a get_or_create pattern: when
ifPhysAddress is present, attempt to get_or_create the MACAddress (use
MACAddress.objects.get_or_create(mac_address=ifPhysAddress)) and then add the
returned instance to interface.mac_addresses; keep the prior check for
existing_mac optional (you can simply use the get_or_create result) so duplicate
MAC rows won’t cause failures during sync.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
.devcontainer/README.md.devcontainer/scripts/setup.shdocs/usage_tips/permissions.mdnetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/settings_views.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.py
🧰 Additional context used
📓 Path-based instructions (6)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Modals use Tabler (Bootstrap-like) but withoutbootstrap.Modalhelpers.
Modal buttons should target thehtmx-modal-contentelement. JavaScript inlibrenms_import.htmltoggles the wrapper; do not reintroducedata-bs-toggleor duplicate modal IDs.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up with TomSelect decorators.
Styling assumes Tabler defaults. Do not addtable-responsivewrappers as they were deliberately removed to prevent dropdown clipping.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Templates should live in
templates/netbox_librenms_plugin/. Reusable template includes should be placed underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/*sync*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Sync pages should extend
librenms_sync_base.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuse thelibrenms_api.pyLibreNMS client instead of making newrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching
Always callLibreNMSAPI.get_librenms_idto retrieve the device/VM LibreNMS mapping via thelibrenms_idcustom field instead of touching the field directly
Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functionsfind_matching_site,match_librenms_hardware_to_device_type, andfind_matching_platformfromutils.py
Files:
netbox_librenms_plugin/api/views.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/settings_views.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/interfaces.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: Use Job UUID (job.job_id) for RQ API endpoints:/api/core/background-tasks/{uuid}/
Use Job PK (job.pk) for database endpoints and result loading
Checkrq_job.is_stoppedorrq_job.is_failedflags in Redis for cancellation detection, not database status
Call/api/core/background-tasks/{uuid}/stop/to stop RQ job in the job cancellation flow
Call plugin's sync endpoint/api/plugins/librenms_plugin/jobs/{pk}/sync-status/to update database after stopping RQ job
Poll/api/core/background-tasks/{uuid}/for real-time RQ status instead of polling the database
api/views.py::sync_job_status()must sync database Job status with RQ job status because NetBox worker doesn't always update DB when jobs stop before processing starts
Files:
netbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/imports/actions.py
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments should live in
templates/netbox_librenms_plugin/htmx/. Keep server responses and HTMX targets in sync.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
🧠 Learnings (31)
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/netbox_librenms_plugin/*sync*.html : Sync pages should extend `librenms_sync_base.html`.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmldocs/usage_tips/permissions.mdnetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Avoid `outerHTML` swaps in HTMX; use OOB or targeted `innerHTML` swaps to keep table layout intact.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to templates/**/*.html : Reuse template includes under `templates/netbox_librenms_plugin/inc/` and ensure sync pages extend `librenms_sync_base.html`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Styling assumes Tabler defaults. Do not add `table-responsive` wrappers as they were deliberately removed to prevent dropdown clipping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : HTMX 2.x is the primary async layer. Table row updates should return `<tr hx-swap-oob="true">`.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Keep `<select class="device-role-select">` markup stable to preserve JavaScript hook-up with TomSelect decorators.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/**/*.html : HTMX fragments should live in `templates/netbox_librenms_plugin/htmx/`. Keep server responses and HTMX targets in sync.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modal buttons should target the `htmx-modal-content` element. JavaScript in `librenms_import.html` toggles the wrapper; do not reintroduce `data-bs-toggle` or duplicate modal IDs.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Applies to netbox_librenms_plugin/templates/**/*.html : Modals use Tabler (Bootstrap-like) but without `bootstrap.Modal` helpers.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to {navigation.py,urls.py,api/**} : Respect NetBox plugin APIs in `navigation.py`, `urls.py`, and `api/` directories
Applied to files:
netbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
Applied to files:
netbox_librenms_plugin/api/views.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : `api/views.py::sync_job_status()` must sync database Job status with RQ job status because NetBox worker doesn't always update DB when jobs stop before processing starts
Applied to files:
netbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/imports/list.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : Call plugin's sync endpoint `/api/plugins/librenms_plugin/jobs/{pk}/sync-status/` to update database after stopping RQ job
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/jobs.py : Jobs must run via Redis Queue (RQ) in Redis, separate from the database Job model, and real-time status must be checked via RQ, not the database
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : Use Job UUID (`job.job_id`) for RQ API endpoints: `/api/core/background-tasks/{uuid}/`
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/jobs.py : Background jobs must use NetBox's `JobRunner` base class (`netbox.jobs.JobRunner`) for long-running operations like device filtering with VC detection
Applied to files:
netbox_librenms_plugin/api/views.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:39:03.395Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/background-jobs.instructions.md:0-0
Timestamp: 2026-02-15T15:39:03.395Z
Learning: Applies to **/views/imports/** : Poll `/api/core/background-tasks/{uuid}/` for real-time RQ status instead of polling the database
Applied to files:
netbox_librenms_plugin/api/views.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/**/*.py : Test coverage mapping: `librenms_api.py` → `test_librenms_api.py`, `import_utils.py`/`import_validation_helpers.py`/`utils.py` → `test_import_utils.py`/`test_import_validation_helpers.py`/`test_utils.py`, `jobs.py`/`views/imports/list.py` → `test_background_jobs.py`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
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
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_import_utils.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_api.py
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_background_jobs.py : Patch `cache` where imported: `netbox_librenms_plugin.views.imports.list.cache`, not `django.core.cache.cache`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/cables.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to **/*.py : Always call `LibreNMSAPI.get_librenms_id` to retrieve the device/VM LibreNMS mapping via the `librenms_id` custom field instead of touching the field directly
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to **/*.py : Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functions `find_matching_site`, `match_librenms_hardware_to_device_type`, and `find_matching_platform` from `utils.py`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Follow sync pipeline flow: fetch LibreNMS data via `librenms_api.py`, cache it using `CacheMixin`, build comparison tables in `tables/`, and render HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Applied to files:
netbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to tables/**/*.py : Prefer updating the table renderer in `tables/*.py` rather than templates when changing row actions, since tables emit HTMX-enabled columns and buttons
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-02-15T15:39:11.571Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/frontend.instructions.md:0-0
Timestamp: 2026-02-15T15:39:11.571Z
Learning: Tables drive most UIs via `tables/*.py` renderers that emit HTMX-enabled columns and buttons. Prefer updating the table renderer in Python rather than templates when changing row actions.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to **/*.py : Reuse the `librenms_api.py` LibreNMS client instead of making new `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and the `servers` plugin config, plus caching
Applied to files:
netbox_librenms_plugin/forms.py
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to {jobs.py,views/imports/**/*.py} : Background job files and import views follow conventions documented in `.github/instructions/background-jobs.instructions.md`
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:39:20.734Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.734Z
Learning: Applies to tests/test_background_jobs.py : Test view decision logic by setting `view._filter_form_data = {...}` directly, not via HTTP requests
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
📚 Learning: 2026-02-15T15:38:54.168Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.168Z
Learning: Applies to views/**/*.py : Views should follow layered structure: extend the closest base class from `views/base/` and compose mixins like `LibreNMSAPIMixin` and `CacheMixin`
Applied to files:
netbox_librenms_plugin/views/sync/cables.py
🧬 Code graph analysis (8)
netbox_librenms_plugin/api/views.py (2)
netbox_librenms_plugin/models.py (1)
InterfaceTypeMapping(51-76)netbox_librenms_plugin/api/serializers.py (1)
InterfaceTypeMappingSerializer(6-13)
netbox_librenms_plugin/views/settings_views.py (4)
netbox_librenms_plugin/forms.py (1)
ImportSettingsForm(114-211)netbox_librenms_plugin/views/mixins.py (2)
LibreNMSPermissionMixin(28-82)require_write_permission(43-65)netbox_librenms_plugin/models.py (1)
LibreNMSSettings(7-48)netbox_librenms_plugin/utils.py (1)
_save_user_pref(152-158)
netbox_librenms_plugin/forms.py (1)
netbox_librenms_plugin/librenms_api.py (1)
get_poller_groups(678-709)
netbox_librenms_plugin/utils.py (1)
netbox_librenms_plugin/views/imports/list.py (1)
get(124-343)
netbox_librenms_plugin/views/imports/actions.py (6)
netbox_librenms_plugin/utils.py (2)
save_import_toggle_prefs(161-176)_save_user_pref(152-158)netbox_librenms_plugin/views/mixins.py (4)
LibreNMSAPIMixin(196-282)LibreNMSPermissionMixin(28-82)require_write_permission(43-65)librenms_api(217-231)netbox_librenms_plugin/views/settings_views.py (3)
post(45-112)post(121-184)get(26-43)netbox_librenms_plugin/views/sync/ip_addresses.py (1)
post(64-86)netbox_librenms_plugin/views/imports/list.py (1)
get(124-343)netbox_librenms_plugin/import_utils.py (1)
_determine_device_name(230-283)
netbox_librenms_plugin/tests/test_librenms_api.py (2)
netbox_librenms_plugin/tests/test_librenms_api_helpers.py (1)
mock_librenms_config(9-26)netbox_librenms_plugin/librenms_api.py (1)
add_device(363-427)
netbox_librenms_plugin/tables/interfaces.py (3)
netbox_librenms_plugin/tables/ipaddresses.py (1)
Meta(18-36)netbox_librenms_plugin/tables/device_status.py (2)
Meta(55-82)Meta(672-712)netbox_librenms_plugin/tables/mappings.py (1)
Meta(18-38)
netbox_librenms_plugin/views/sync/interfaces.py (3)
netbox_librenms_plugin/views/mixins.py (4)
LibreNMSPermissionMixin(28-82)NetBoxObjectPermissionMixin(85-193)require_all_permissions(167-179)require_all_permissions_json(181-193)netbox_librenms_plugin/utils.py (1)
convert_speed_to_kbps(12-24)netbox_librenms_plugin/models.py (1)
InterfaceTypeMapping(51-76)
🪛 LanguageTool
docs/usage_tips/permissions.md
[style] ~152-~152: Consider an alternative verb to strengthen your wording.
Context: ...ound job APIs should not appear. If you see these errors, it usually means a direct...
(IF_YOU_HAVE_THIS_PROBLEM)
🔇 Additional comments (37)
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (2)
159-165: LGTM — Clean empty-state fallback.The no-data card is well-structured and consistent with the same pattern applied to cable and IP address sync content templates. Good use of the
mdi-sync-officon and clear instructional text.
303-303: No concerns — EOF newline normalization.netbox_librenms_plugin/tests/test_librenms_api.py (3)
592-618: LGTM — SNMPv1 payload validation is thorough.The test correctly verifies the
snmpvermapping and presence ofcommunityin the outgoing POST payload.
620-641: Correct adjustment — duplicate returns HTTP 200 with error in body.Changing the mock to
status_code = 200aligns with real LibreNMS API behavior where duplicates return200with"status": "error"in JSON. Sinceraise_for_status()on aMagicMockis a no-op by default, the test works, but it's worth noting this relies on implicit MagicMock behavior rather than explicitly stubbingraise_for_status.
643-681: LGTM — SNMPv3 test covers all fields and the negative assertion.Good coverage: validates all six v3 credential fields in the payload and confirms
communityis absent..devcontainer/scripts/setup.sh (1)
74-76: Good addition for isolated virtualenv compatibility.Setting
pip config set global.certensures tools likepre-committhat create their own virtualenvs will use the system CA bundle rather than their bundledcertifi. The|| truefallback is appropriate.netbox_librenms_plugin/tables/interfaces.py (2)
55-57: PreviousAttributeErrorfix properly addressed.The
str()coercion before.lower()correctly handles non-stringifAdminStatusvalues, consistent with the_parse_enabled_statusguard at line 131.
23-25: Docstring additions are consistent with the rest of the codebase.The new docstrings on
Metaclasses and__init__methods align with the pattern used indevice_status.py,mappings.py, andipaddresses.py.Also applies to: 42-43, 322-323, 362-363, 368-370, 393-395
netbox_librenms_plugin/api/views.py (3)
21-32: Clean permission class implementation.Standard DRF pattern with
SAFE_METHODSgating. Docstring now correctly shows full permission strings.
44-45: Good migration to DRF decorators.Using
@api_view(["POST"])with@permission_classesis cleaner than manual HTTP method guards and provides proper DRF response negotiation. Based on learnings: "REST endpoints for imports live inviews/imports/actions.py... surface viaurls.py" — this endpoint correctly uses the DRF decorator pattern consistent with that architecture.
35-41: Remove this review comment. The implementation is correct and follows the plugin's documented permission system.Per the plugin's architectural guidelines, API endpoints must use
LibreNMSPluginPermissionexclusively—not composed withTokenPermissions. The plugin implements a two-tier permission model (view_librenmssettingsfor reads,change_librenmssettingsfor writes) as the single source of truth for all API access, which is the documented and intentional design for plugin configuration models likeInterfaceTypeMapping.Likely an incorrect or invalid review comment.
netbox_librenms_plugin/views/sync/cables.py (6)
13-21: Permission mixin composition and declaration look good.The MRO places the permission mixins before
CacheMixinandView, ensuring permission checks run first. Therequired_object_permissionsmapping for POST requiring bothaddandchangeonCableis appropriate since the sync both creates new cables and modifies termination state.
82-88: StopIteration handler now correctly includes the interface identifier — looks good.
100-117: Cable creation flow is well-structured.The guard → lookup → duplicate-check → create pipeline is clean, and both interface lookups are properly covered by the single
DoesNotExisthandler.
119-132: Per-interface atomic blocks provide good failure isolation.Each cable creation rolls back independently on failure. The result aggregation correctly maps to the four predefined status buckets.
134-154: Clean POST flow with early-exit permission and validation gates.The sequential check pattern (permissions → prerequisites → processing → results) is easy to follow and ensures no work is done until all preconditions pass.
156-177: Result messaging is well-categorized by severity.Appropriate use of
error,warning, andsuccesslevels for each outcome type..devcontainer/README.md (1)
162-162: LGTM — grammar fix applied as previously suggested.netbox_librenms_plugin/utils.py (2)
161-176: LGTM — toggle persistence logic is correct.Absent checkbox values correctly result in
Falsebeing saved, matching the documented behavior forhx-include.
192-207: LGTM — redundant write avoidance and preference cascade are well implemented.The check at line 197 prevents unnecessary DB writes on GET, and the fallback chain (request param → user pref → plugin config) is clean.
netbox_librenms_plugin/views/settings_views.py (3)
141-143: Good security improvement: HTML-escaping dynamic API response data.Using
escape()on all externally-sourced values before interpolating them into HTML fragments prevents XSS.
47-49: LGTM — write permission guard on POST.Clean walrus-operator pattern for early return on permission denial.
77-90: LGTM — defensive try/except around preference persistence.Addresses the previous review concern. Settings save is not affected if user pref update fails.
netbox_librenms_plugin/forms.py (1)
282-339: LGTM — form renamed and poller group choices delegated to shared helper.The
AddToLIbreSNMPV1V2form correctly removes thesnmp_versionfield (now handled by the view) and uses the cached helper for poller group choices.netbox_librenms_plugin/views/sync/devices.py (2)
24-29: LGTM —get_object_or_404prevents uncaughtVirtualMachine.DoesNotExist.Properly returns a 404 instead of a 500 when neither Device nor VirtualMachine exists.
104-134: LGTM — permission guard and API usage are correct.Write permission is checked before modifying LibreNMS data, and
get_librenms_idis used per coding guidelines.netbox_librenms_plugin/views/imports/list.py (2)
310-317: LGTM — user preference overrides with correct model-aligned fallbacks.
use_sysnamefalls back toTrue(matchingLibreNMSSettings.use_sysname_default) andstrip_domainfalls back toFalse(matchingLibreNMSSettings.strip_domain_default). This addresses the prior review concern.
54-63: LGTM — non-superuser guard for background jobs.Correctly prevents non-superusers from using background jobs, as the
/api/core/background-tasks/endpoint requires superuser access. As per coding guidelines, this ensures appropriate fallback to synchronous processing.netbox_librenms_plugin/views/sync/device_fields.py (4)
43-52: LGTM —full_clean()+save()with rollback on failure.The
old_namecapture (line 43), validation viafull_clean(), and revert on exception properly address the previous review concern about unhandled uniqueness/validation errors.
280-303: Good use oftransaction.atomic()for platform creation + assignment.The atomic block ensures the newly created Platform is rolled back if
device.full_clean()ordevice.save()fails. The slug collision detection at line 292 is a pragmatic approach for user-facing messages, though it relies on string matching against the exception text.
352-362: LGTM — per-member serial validation with rollback.
old_serialis now captured before overwriting (line 352), and errors are per-member without aborting the entire batch. This addresses the previous review feedback.
15-17: Therequire_all_permissionsmethod is correctly implemented onLibreNMSPermissionMixinand verifies both plugin-level write permission and per-viewrequired_object_permissions.The method exists at
netbox_librenms_plugin/views/mixins.py:167and chainsrequire_write_permission()(which checksPERM_CHANGE_PLUGIN) withrequire_object_permissions(method)(which checks therequired_object_permissionsdict). All calling views properly define the required permissions dict. No action needed.netbox_librenms_plugin/views/sync/interfaces.py (2)
16-37: Dynamic permission mapping looks solid.
Object-type-specific permissions plusrequire_all_permissionsbefore sync is a clean, consistent gate.
225-246: Delete flow permission guard is consistent.
Dynamic permission mapping +require_all_permissions_jsonkeeps delete aligned with the sync permission model.netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html (2)
203-219: Clear fallback when background jobs are unavailable.
Disabled control plus explanatory copy makes the synchronous fallback explicit.
436-466: Preference persistence JS is robust.
CSRF guard + same‑origin request + error logging improves resiliency without blocking UI.netbox_librenms_plugin/views/imports/actions.py (1)
210-218: Write-permission gate before bulk confirm is good.
Consistent with other POST endpoints and prevents unauthorized bulk actions early.
| mkdir -p /usr/local/share/ca-certificates/proxy | ||
| # Remove stale split fragments so they don't accumulate across rebuilds | ||
| find /usr/local/share/ca-certificates/proxy -maxdepth 1 -name 'cert-*' -delete 2>/dev/null || true | ||
| # Split the bundle into individual certs — update-ca-certificates needs one | ||
| # cert per file and skips non-CA leaf certs, so extract each PEM block as | ||
| # a separate .crt file. | ||
| csplit -z -f /usr/local/share/ca-certificates/proxy/cert- \ | ||
| "$CA_BUNDLE_SRC" '/-----BEGIN CERTIFICATE-----/' '{*}' \ | ||
| >/dev/null 2>&1 | ||
| # Rename split fragments to .crt | ||
| for f in /usr/local/share/ca-certificates/proxy/cert-*; do | ||
| mv "$f" "${f}.crt" 2>/dev/null || true | ||
| done |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Past review feedback properly addressed — stale fragment cleanup looks good.
The find … -delete on Line 56 clears old fragments before re-splitting, and the overall flow is sound.
One minor robustness nit: if the glob on Line 64 matches nothing (e.g., csplit produced zero output), bash will pass the literal string cert-* to mv, which fails silently thanks to || true. Consider enabling nullglob locally to avoid the unnecessary error path:
Optional hardening
# Rename split fragments to .crt
+ shopt -s nullglob
for f in /usr/local/share/ca-certificates/proxy/cert-*; do
mv "$f" "${f}.crt" 2>/dev/null || true
done
+ shopt -u nullglob🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/scripts/setup.sh around lines 54 - 66, Enable bash's nullglob
around the cert-moving loop to avoid iterating over the literal pattern when no
files were created: locally set nullglob before the for f in
/usr/local/share/ca-certificates/proxy/cert-* loop that renames fragments to
.crt, run the mv commands as-is, then restore the previous shell option after
the loop; reference the for loop that processes
/usr/local/share/ca-certificates/proxy/cert-* and the CA_BUNDLE_SRC csplit step
when making the change.
| - Change: allows performing actions that modify Netbox or Librenms data | ||
|
|
||
| The Plugin also enforces Netbox object permissions so the following permission would also be required: | ||
|
|
||
| 1. **Tier 2: Object permission**: User needs `dcim.add_device` (to create the device in NetBox) | ||
|
|
||
| If either permission is missing, the operation fails with an appropriate error message. | ||
|
|
||
| ## Creating Permissions | ||
|
|
||
| All permissions are created using Netbox's standard Object permissions UI. |
There was a problem hiding this comment.
Inconsistent capitalization: "Netbox" vs. "NetBox".
Lines 25, 27, and 35 use "Netbox" while the rest of the document (and the official name) uses "NetBox". Same issue appears on line 94.
Proposed fix
- - Change: allows performing actions that modify Netbox or Librenms data
+ - Change: allows performing actions that modify NetBox or LibreNMS data
-The Plugin also enforces Netbox object permissions so the following permission would also be required:
+The Plugin also enforces NetBox object permissions so the following permission would also be required:-All permissions are created using Netbox's standard Object permissions UI.
+All permissions are created using NetBox's standard Object permissions UI.Also on line 94:
-Users without View permission won't see the LibreNMS menu or the LibreNMS Sync tab. Users with **View** but not **Change** can browse all plugin pages but cannot perform import or sync actions that modify Netbox data and Librenms data like Locations and Adding devices.
+Users without View permission won't see the LibreNMS menu or the LibreNMS Sync tab. Users with **View** but not **Change** can browse all plugin pages but cannot perform import or sync actions that modify NetBox data and LibreNMS data like Locations and Adding devices.📝 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.
| - Change: allows performing actions that modify Netbox or Librenms data | |
| The Plugin also enforces Netbox object permissions so the following permission would also be required: | |
| 1. **Tier 2: Object permission**: User needs `dcim.add_device` (to create the device in NetBox) | |
| If either permission is missing, the operation fails with an appropriate error message. | |
| ## Creating Permissions | |
| All permissions are created using Netbox's standard Object permissions UI. | |
| - Change: allows performing actions that modify NetBox or LibreNMS data | |
| The Plugin also enforces NetBox object permissions so the following permission would also be required: | |
| 1. **Tier 2: Object permission**: User needs `dcim.add_device` (to create the device in NetBox) | |
| If either permission is missing, the operation fails with an appropriate error message. | |
| ## Creating Permissions | |
| All permissions are created using NetBox's standard Object permissions UI. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/usage_tips/permissions.md` around lines 25 - 35, Replace the
inconsistent "Netbox" capitalization with the correct "NetBox" throughout the
document—specifically change the occurrences shown in the diff (the sentence
beginning "allows performing actions that modify Netbox or Librenms data", the
heading/note referencing "Netbox object permissions" and the line mentioning
"The Plugin also enforces Netbox object permissions") and the additional
occurrence near the later mention (search for the literal "Netbox" to find the
remaining instance); keep the surrounding text (e.g., "Tier 2: Object
permission" and "dcim.add_device") unchanged.
| def _get_librenms_poller_group_choices(): | ||
| """ | ||
| Helper function to get poller group choices from LibreNMS API. | ||
| Shared between AddToLIbreSNMPV1V2 and AddToLIbreSNMPV3 forms. | ||
| Results are cached to avoid repeated API calls on every form instantiation. | ||
| """ | ||
| from django.core.cache import cache | ||
|
|
||
| from .librenms_api import LibreNMSAPI | ||
|
|
||
| choices = [("0", "Default (0)")] | ||
|
|
||
| cache_key = "librenms_poller_group_choices" | ||
| cached_choices = cache.get(cache_key) | ||
| if cached_choices: | ||
| return cached_choices | ||
|
|
||
| try: | ||
| api = LibreNMSAPI() | ||
| success, poller_groups = api.get_poller_groups() | ||
|
|
||
| if success and poller_groups: | ||
| for group in poller_groups: | ||
| group_id = str(group.get("id", "")) | ||
| group_name = group.get("group_name", "") | ||
| group_descr = group.get("descr", "") | ||
|
|
||
| if group_id: | ||
| if group_descr and group_descr != group_name: | ||
| label = f"{group_name} - {group_descr} ({group_id})" | ||
| else: | ||
| label = f"{group_name} ({group_id})" | ||
| choices.append((group_id, label)) | ||
|
|
||
| cache.set(cache_key, choices, timeout=api.cache_timeout) | ||
| except Exception: | ||
| logger.exception("Failed to load LibreNMS poller groups") | ||
|
|
||
| return choices |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good: caching and proper error logging address previous review feedback.
One minor observation: if cached_choices happened to be an empty list [], the if cached_choices: check on line 64 would be falsy, causing repeated API calls. In practice this won't happen because the default choice is always included before caching. However, using if cached_choices is not None: would be more defensive.
Optional defensive fix
cached_choices = cache.get(cache_key)
- if cached_choices:
+ if cached_choices is not None:
return cached_choices🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/forms.py` around lines 50 - 88, The cached_choices
truthiness check in _get_librenms_poller_group_choices can misinterpret an
intentionally cached empty list as a cache miss; update the conditional that
reads cached_choices (obtained via cache.get(cache_key)) to check for None
explicitly (e.g., cached_choices is not None) so an empty list is returned from
cache instead of triggering another API call; keep the rest of the logic
(including cache.set(cache_key, choices, timeout=api.cache_timeout) and the
default ("0","Default (0)") choice) unchanged.
| def get_cached_links_data(self, request, obj): | ||
| """Return cached LibreNMS link data for the given object.""" | ||
| cached_data = cache.get(self.get_cache_key(obj, "links")) | ||
| if not cached_data: | ||
| return None | ||
| return cached_data.get("links", []) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused request parameter.
request is accepted but never referenced; only obj is used. Drop it from the signature and the call site (line 142) to avoid confusion.
Proposed fix
- def get_cached_links_data(self, request, obj):
+ def get_cached_links_data(self, obj):
"""Return cached LibreNMS link data for the given object."""And at the call site (line 142):
- cached_links = self.get_cached_links_data(request, initial_device)
+ cached_links = self.get_cached_links_data(initial_device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/cables.py` around lines 37 - 42, The method
get_cached_links_data accepts an unused request parameter; remove the request
parameter from its signature and docstring, update its body to use only obj
(keep the cache.get(self.get_cache_key(obj, "links")) logic and return
behavior), and update every call site that currently passes a request to instead
pass only the obj when invoking get_cached_links_data so signatures match.
Resolve interface_name_field at request time in get_links_data() instead of as a class-level attribute. The class attribute called get_interface_name_field() without a request object, so it always defaulted to ifName regardless of user preference.
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
2 participants
Enhance import validation modal and sync page alignment
Add serial number matching, conflict resolution, and import improvements
Summary by CodeRabbit
Release Notes
New Features
Improvements