fix: develop-era hardening pass (security + correctness + tests) - #116
Merged
Conversation
📝 WalkthroughWalkthroughThe PR hardens LibreNMS server selection, payload validation, import matching, permissions, rendering, caching, and synchronization. It adds normalization migration logic, live API reads, HTMX updates, accessibility changes, and extensive database-backed regression coverage. ChangesLibreNMS hardening and synchronization
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
543-560:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreloaded normalization rules are bypassed in chassis fallback matching
The primary hardware match now uses preloaded rules, but the chassis fallback path still calls matching without
preloaded_rules. In bulk imports, unmatched hardware can still trigger per-device NormalizationRule queries through that fallback, partially reintroducing the N+1 cost.♻️ Suggested fix
-def _try_chassis_device_type_match(api, device_id): +def _try_chassis_device_type_match(api, device_id, preloaded_device_type_rules: dict | None = None): @@ - chassis_match = match_librenms_hardware_to_device_type(value) + chassis_match = match_librenms_hardware_to_device_type( + value, + preloaded_rules=preloaded_device_type_rules, + ) @@ - chassis_match = _try_chassis_device_type_match(api, device_id) + chassis_match = _try_chassis_device_type_match( + api, + device_id, + preloaded_device_type_rules=preloaded_device_type_rules, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 543 - 560, The chassis fallback matching path uses _try_chassis_device_type_match without passing the preloaded normalization rules, which causes additional per-device database queries during bulk imports. Pass the preloaded_device_type_rules parameter to the _try_chassis_device_type_match function call (where it's invoked in the else block after checking if dt_match is not None) to ensure the chassis fallback uses the same preloaded rules as the primary hardware matching logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/librenms_api.py`:
- Around line 61-65: The validation logic only ensures a fallback valid
configuration exists but does not validate that the actually selected server
configuration is a dict before attempting to access its keys. Add a validation
check after the server_key is selected (around where servers_config[server_key]
is first accessed) to ensure the selected configuration is a dict before
attempting to read values like librenms_url from it. If the selected server_key
maps to a non-dict value, raise a clear ValueError that indicates the specific
server configuration is malformed rather than allowing a TypeError to occur
during dictionary key access.
In `@netbox_librenms_plugin/views/base/librenms_sync_view.py`:
- Around line 198-205: The code in the validation block that checks
isinstance(did, bool) and did is None does not validate the type of did before
calling int(did), which allows float values to be silently truncated. Add an
additional type check after the bool and None checks to ensure did is only of
type str or int before attempting the int() conversion, rejecting other types
like float. This ensures consistency with the type validation approach used in
get_librenms_device_id() in utils.py.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 543-560: The chassis fallback matching path uses
_try_chassis_device_type_match without passing the preloaded normalization
rules, which causes additional per-device database queries during bulk imports.
Pass the preloaded_device_type_rules parameter to the
_try_chassis_device_type_match function call (where it's invoked in the else
block after checking if dt_match is not None) to ensure the chassis fallback
uses the same preloaded rules as the primary hardware matching logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0f4158d-12f3-4570-9c27-bfb21fb828d9
📒 Files selected for processing (26)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.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/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/sync/interfaces.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.12)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/interfaces_view.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/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 live in
templates/netbox_librenms_plugin/htmx/including:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/interfaces.py
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust export:validate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/interfaces.py
🧠 Learnings (36)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_utils.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/base/interfaces_view.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/base/interfaces_view.py
📚 Learning: 2026-06-01T15:12:26.824Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/ip_addresses.py:94-103
Timestamp: 2026-06-01T15:12:26.824Z
Learning: For any redirect/tab URL building in netbox_librenms_plugin/views/sync, views/base, and views/object_sync, propagate the active multi-server `server_key` as a `?server_key=<key>` query parameter so users return to the same server’s tab after POST actions. When handling POST requests, read the POST-scoped `server_key` from `request.POST` and store it (e.g., `self._post_server_key`) with a fallback to `self.librenms_api.server_key`; use this POST-scoped key for both cache-key scoping and for constructing the redirect/tab URLs. Treat this as the intentional codebase-wide convention—do not flag the presence/usage of the `server_key` query parameter (or the corresponding POST-scoped `_post_server_key` pattern) in these views as an error.
Applied to files:
netbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/base/interfaces_view.py
📚 Learning: 2026-06-05T07:19:49.079Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/base/interfaces_view.py:158-165
Timestamp: 2026-06-05T07:19:49.079Z
Learning: When building OOB relationships from interface/device view code, call get_librenms_oob() using the resolved sync device (e.g., `lookup_device = get_librenms_sync_device(obj, server_key=...) or obj; oob = get_librenms_oob(lookup_device, ...)`) rather than calling get_librenms_oob(obj, ... ) directly. For VC members, OOB data (including shared-LOM markers) is stored on the resolved sync device, so resolving first is required to avoid dropping OOB rows.
Applied to files:
netbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/interfaces_view.py
📚 Learning: 2026-03-07T09:14:06.791Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:324-331
Timestamp: 2026-03-07T09:14:06.791Z
Learning: In netbox_librenms_plugin/views/base/cables_view.py, do not treat cache.ttl() usage as a portability issue. NetBox requires Redis as the cache backend (since NetBox v2.6), so django-redis cache.ttl() and cache.pttl() extensions are available. Consider this as a project-specific guideline: cache.ttl() is intentional/safe in this codebase.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-03-09T22:06:30.733Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_sync_view_mismatch.py:405-448
Timestamp: 2026-03-09T22:06:30.733Z
Learning: In netbox_librenms_plugin/tests/test_sync_view_mismatch.py, do not re-raise the PyFlakes F811 redefinition warning for the duplicate test_vc_member_with_legacy_id_delegates_to_sync_device in class TestVCLookupDelegation. This has been resolved in commit 2e52600 (PR `#245`) by merging duplicates into a single class with all distinct tests. Treat this file as already corrected; do not flag F811 for this specific issue.
Applied to files:
netbox_librenms_plugin/tests/test_sync_view_mismatch.py
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-05-25T21:48:19.264Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html:12-12
Timestamp: 2026-05-25T21:48:19.264Z
Learning: In this plugin’s HTMX form templates, `hx-include` selectors that target toggle preference wrapper `<span>` element IDs (e.g., `#use-sysname-toggle`, `#strip-domain-toggle`, `#auto-create-ipam-toggle`) are intentional. Those wrapper spans contain both the hidden `off` fallback input and the checkbox; HTMX must include the wrapper so the correct value is serialized, including the unchecked/off state. Do not recommend changing `hx-include` to the checkbox IDs with the `-cb` suffix, since it would omit the hidden fallback and break unchecked/off state submission.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-03-08T19:01:30.947Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/views/base/librenms_sync_view.py:36-41
Timestamp: 2026-03-08T19:01:30.947Z
Learning: In netbox_librenms_plugin/views/base/librenms_sync_view.py, when checking for a stored librenms_id, use explicit None-check instead of a bare truthiness test. Specifically, in get_context_data(), prefer: self.librenms_api.get_librenms_id(librenms_sync_device) is not None rather than a boolean conversion. This ensures a stored value of 0 is treated as present (since 0 is falsy in Python) and avoids misclassifying it as missing. Do not replace with bool(...) or a plain if self.librenms_api.get_librenms_id(...) check. Rationale: differentiates between None (no mapping) and valid numeric ids (including 0).
Applied to files:
netbox_librenms_plugin/views/base/librenms_sync_view.py
📚 Learning: 2026-03-07T10:40:38.106Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:241-244
Timestamp: 2026-03-07T10:40:38.106Z
Learning: In netbox_librenms_plugin/views/sync/interfaces.py, ensure that set_librenms_device_id does not apply the legacy bare-integer guard to Interface/VMInterface objects. The guard is only relevant for Device/VM objects with pre-existing bare integers from before multi-server support. Interfaces/VMInterfaces have librenms_id starting empty and their port_id is always written from the LibreNMS API JSON response, so there is no migration concern. Do not treat the warning-log path as a silent no-op for interfaces; keep appropriate logging/alerts active. Add or adjust tests to verify that interfaces paths write port_id correctly and do not trigger the legacy-bare-int logic, and document this distinction in code comments.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-05-22T19:37:46.167Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_import_utils.py:1763-1766
Timestamp: 2026-05-22T19:37:46.167Z
Learning: In `validate_device_for_import` (device operations / serial-diff name-resolution logic), preserve the following result contract so the UI/test expectations remain stable:
- If `serial_action` is determined via a serial match AND the existing device has NO OOB/LibreNMS link, set `serial_action` to `"oob_candidate"` and ensure `promote_to_host` is NOT present in the returned dict.
- Populate `promote_to_host` only when the existing device already has an OOB/LibreNMS link (i.e., a host id is available to inherit from); otherwise omit the key.
- Always include `serial_role_choice_available` in the returned dict, defaulting to `False` (baseline) even when other resolution outcomes do not enable it.
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-12T20:27:53.873Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/librenms_api.py:703-709
Timestamp: 2026-03-12T20:27:53.873Z
Learning: In netbox_librenms_plugin/librenms_api.py, enforce that get_device_inventory() and get_inventory_filtered() always return a list of dicts. Validate as: inventory must be a list and every item must be a dict; if not, log a warning with the raw payload and return (False, error_message). Do not weaken the check to just verify a list type. This should prevent downstream AttributeError/TypeError when callers call .get() on items.
Applied to files:
netbox_librenms_plugin/librenms_api.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/interfaces.py
📚 Learning: 2026-03-07T10:32:06.242Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:107-117
Timestamp: 2026-03-07T10:32:06.242Z
Learning: In netbox_librenms_plugin/utils.py, keep the Priority 1 loop to guard against None and bool values when accessing raw_cf.get(server_key). Do not replace the Priority 1 condition with a full get_librenms_device_id call. The two-pass design is intentional: Priority 1 performs quick sanity checks, while Priority 2 handles string normalization and full validation by calling get_librenms_device_id(member, server_key, auto_save=False).
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-07T22:38:43.110Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:552-584
Timestamp: 2026-03-07T22:38:43.110Z
Learning: In netbox_librenms_plugin/utils.py, do not propose replacing 'obj.custom_field_data.get("librenms_id") or {}' with a None check. The code intentionally uses 'or {}' to handle falsey values; downstream type guards treat them equivalently since LibreNMS IDs start at 1, making 0 equivalent to 'not set'. Do not modify this logic; keep the existing behavior for all falsey values.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T08:55:46.317Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:46.317Z
Learning: In netbox_librenms_plugin/utils.py, do not modify set_librenms_device_id to call obj.save(). It is mutator-only and should only update in-memory obj.custom_field_data[...] without persisting. Ensure callers perform persistence: after mutation, run full_clean() and then save() (as seen in device_operations.py around lines ~864-866) or explicit obj.save() after set_librenms_device_id (as in librenms_api.py around lines ~261-262). This pattern prevents coupling mutation with persistence and preserves validation in between.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T19:15:13.104Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:249-267
Timestamp: 2026-03-09T19:15:13.104Z
Learning: In netbox_librenms_plugin/utils.py, ensure match_librenms_hardware_to_device_type returns None when DeviceTypeMapping.MultipleObjectsReturned is raised (fail-closed per inline comment). Callers must guard for result is None separately from the normal result check (e.g., if result is None: handle; elif result.get('matched'): ... ). Note that the success path uses match_type='mapping' (not 'exact'), distinguishing it from standard part_number/model exact lookups. Consider adding a unit test that asserts None is returned on MultipleObjectsReturned and that callers properly handle both None and dict results.
Applied to files:
netbox_librenms_plugin/utils.py
🪛 ast-grep (0.43.0)
netbox_librenms_plugin/views/sync/interfaces.py
[error] 72-72: Filename coming from the request
Context: request.POST.get("server_key")
Note: [CWE-22].
(open-filename-from-request)
🪛 OpenGrep (1.22.0)
netbox_librenms_plugin/tables/interfaces.py
[WARNING] 185-185: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
[WARNING] 189-189: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
[WARNING] 205-205: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (25)
netbox_librenms_plugin/import_utils/bulk_import.py (1)
11-11: LGTM!Also applies to: 120-123, 163-163
netbox_librenms_plugin/import_utils/device_operations.py (1)
138-139: LGTM!Also applies to: 447-458
netbox_librenms_plugin/tests/test_coverage_utils.py (1)
5-6: LGTM!Also applies to: 325-327, 379-457
netbox_librenms_plugin/tests/test_utils.py (1)
11-22: LGTM!netbox_librenms_plugin/tests/test_coverage_bulk_import.py (1)
132-140: LGTM!Also applies to: 955-959
netbox_librenms_plugin/tests/test_import_utils.py (1)
10-10: LGTM!Also applies to: 1684-1688, 1706-1738, 1749-1753, 1781-1785, 1813-1817, 1898-1902, 1934-1938, 1983-1987, 2034-2038, 2086-2090, 2144-2148, 2200-2204, 2297-2301, 3679-3688, 4894-4901, 4998-5005
netbox_librenms_plugin/tests/test_permissions.py (1)
636-647: LGTM!Also applies to: 889-898
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
2302-2304: LGTM!Also applies to: 2327-2331, 2344-2347, 2384-2386, 2399-2402
netbox_librenms_plugin/views/sync/interfaces.py (1)
68-78: LGTM!Also applies to: 87-90
netbox_librenms_plugin/tests/test_librenms_api.py (1)
93-133: LGTM!netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py (1)
319-365: LGTM!netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
170-176: LGTM!netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
17-18: LGTM!Also applies to: 111-129, 971-997
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (1)
500-502: LGTM!Also applies to: 551-553
netbox_librenms_plugin/tests/test_validation_template_server_key.py (1)
1-38: LGTM!netbox_librenms_plugin/utils.py (1)
16-25: LGTM!Also applies to: 594-688
netbox_librenms_plugin/views/base/cables_view.py (1)
33-48: LGTM!netbox_librenms_plugin/views/base/interfaces_view.py (1)
10-10: LGTM!Also applies to: 124-130
netbox_librenms_plugin/views/base/vlan_table_view.py (1)
9-9: LGTM!Also applies to: 67-73
netbox_librenms_plugin/views/base/librenms_sync_view.py (1)
258-261: LGTM!netbox_librenms_plugin/tests/test_payload_hardening.py (1)
1-137: LGTM!netbox_librenms_plugin/tests/test_sync_view_mismatch.py (1)
367-384: LGTM!netbox_librenms_plugin/tables/interfaces.py (1)
181-205: LGTM!Also applies to: 293-294, 366-390, 412-437
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
308-309: LGTM!Also applies to: 322-323
netbox_librenms_plugin/tests/test_coverage_tables.py (1)
2652-2694: LGTM!
marcinpsk
commented
Jun 19, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
force-pushed
the
fix/develop-hardening
branch
from
June 20, 2026 05:49
239d1ab to
d19b555
Compare
This was referenced Jun 20, 2026
Merged
Merged
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 420-423: The issue is that int(librenms_id) can lossy coerce float
values like 42.9 into 42, potentially matching an unrelated object. Instead of
allowing this truncation, validate that librenms_id is actually an integer value
(not a float string) before converting it in the call to find_by_librenms_id.
This ensures proper validation fails closed on malformed input rather than
silently binding to an incorrect ID through truncation.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html`:
- Around line 15-24: The two button elements in this template (the ones with
hx-post attributes for "plugins:netbox_librenms_plugin:device_interface_sync"
and "plugins:netbox_librenms_plugin:vm_interface_sync") are missing explicit
type declarations. Add type="button" to both button elements to satisfy HTML
lint rules and prevent implicit form submission behavior, since these are
HTMX-controlled buttons rather than actual form submit buttons.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 238f7e1e-62f7-4761-9bd5-a672103be98f
📒 Files selected for processing (19)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/interfaces.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/utils.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
netbox_librenms_plugin/templates/**/+(*_sync|*_sync_base).html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Sync pages should extend
librenms_sync_base.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
netbox_librenms_plugin/templates/**/_*_sync.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Each sync resource should have two templates:
_<resource>_sync.html(tab wrapper, loaded once) and_<resource>_sync_content.html(HTMX-swappable inner fragment).
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/interfaces.py
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust export:validate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/interfaces.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (35)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/utils.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/utils.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/utils.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
📚 Learning: 2026-03-09T22:06:30.733Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_sync_view_mismatch.py:405-448
Timestamp: 2026-03-09T22:06:30.733Z
Learning: In netbox_librenms_plugin/tests/test_sync_view_mismatch.py, do not re-raise the PyFlakes F811 redefinition warning for the duplicate test_vc_member_with_legacy_id_delegates_to_sync_device in class TestVCLookupDelegation. This has been resolved in commit 2e52600 (PR `#245`) by merging duplicates into a single class with all distinct tests. Treat this file as already corrected; do not flag F811 for this specific issue.
Applied to files:
netbox_librenms_plugin/tests/test_sync_view_mismatch.py
📚 Learning: 2026-03-07T10:40:38.106Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:241-244
Timestamp: 2026-03-07T10:40:38.106Z
Learning: In netbox_librenms_plugin/views/sync/interfaces.py, ensure that set_librenms_device_id does not apply the legacy bare-integer guard to Interface/VMInterface objects. The guard is only relevant for Device/VM objects with pre-existing bare integers from before multi-server support. Interfaces/VMInterfaces have librenms_id starting empty and their port_id is always written from the LibreNMS API JSON response, so there is no migration concern. Do not treat the warning-log path as a silent no-op for interfaces; keep appropriate logging/alerts active. Add or adjust tests to verify that interfaces paths write port_id correctly and do not trigger the legacy-bare-int logic, and document this distinction in code comments.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T15:12:26.824Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/ip_addresses.py:94-103
Timestamp: 2026-06-01T15:12:26.824Z
Learning: For any redirect/tab URL building in netbox_librenms_plugin/views/sync, views/base, and views/object_sync, propagate the active multi-server `server_key` as a `?server_key=<key>` query parameter so users return to the same server’s tab after POST actions. When handling POST requests, read the POST-scoped `server_key` from `request.POST` and store it (e.g., `self._post_server_key`) with a fallback to `self.librenms_api.server_key`; use this POST-scoped key for both cache-key scoping and for constructing the redirect/tab URLs. Treat this as the intentional codebase-wide convention—do not flag the presence/usage of the `server_key` query parameter (or the corresponding POST-scoped `_post_server_key` pattern) in these views as an error.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-03-08T23:14:36.746Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_coverage_sync_view.py:77-100
Timestamp: 2026-03-08T23:14:36.746Z
Learning: In netbox_librenms_plugin/views/base/librenms_sync_view.py, BaseLibreNMSSyncView.get() should include an explicit own-ID guard for VC members: if get_librenms_device_id(obj, server_key, auto_save=False) returns a non-None value (covering both legacy bare-int and per-server dict formats), treat the member as already mapped by keeping self._librenms_lookup_device and do not call get_librenms_sync_device(). Only when get_librenms_device_id(...) is None should you fall through to get_librenms_sync_device(obj, server_key=self.librenms_api.server_key). This guard prevents auto-discovery side-effects on members with an explicit mapping. Do not revert this guard or make delegation unconditional.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_sync_view.py
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-05-22T19:37:46.167Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_import_utils.py:1763-1766
Timestamp: 2026-05-22T19:37:46.167Z
Learning: In `validate_device_for_import` (device operations / serial-diff name-resolution logic), preserve the following result contract so the UI/test expectations remain stable:
- If `serial_action` is determined via a serial match AND the existing device has NO OOB/LibreNMS link, set `serial_action` to `"oob_candidate"` and ensure `promote_to_host` is NOT present in the returned dict.
- Populate `promote_to_host` only when the existing device already has an OOB/LibreNMS link (i.e., a host id is available to inherit from); otherwise omit the key.
- Always include `serial_role_choice_available` in the returned dict, defaulting to `False` (baseline) even when other resolution outcomes do not enable it.
Applied to files:
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/interfaces.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-07T10:32:06.242Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:107-117
Timestamp: 2026-03-07T10:32:06.242Z
Learning: In netbox_librenms_plugin/utils.py, keep the Priority 1 loop to guard against None and bool values when accessing raw_cf.get(server_key). Do not replace the Priority 1 condition with a full get_librenms_device_id call. The two-pass design is intentional: Priority 1 performs quick sanity checks, while Priority 2 handles string normalization and full validation by calling get_librenms_device_id(member, server_key, auto_save=False).
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-07T22:38:43.110Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:552-584
Timestamp: 2026-03-07T22:38:43.110Z
Learning: In netbox_librenms_plugin/utils.py, do not propose replacing 'obj.custom_field_data.get("librenms_id") or {}' with a None check. The code intentionally uses 'or {}' to handle falsey values; downstream type guards treat them equivalently since LibreNMS IDs start at 1, making 0 equivalent to 'not set'. Do not modify this logic; keep the existing behavior for all falsey values.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T08:55:46.317Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:46.317Z
Learning: In netbox_librenms_plugin/utils.py, do not modify set_librenms_device_id to call obj.save(). It is mutator-only and should only update in-memory obj.custom_field_data[...] without persisting. Ensure callers perform persistence: after mutation, run full_clean() and then save() (as seen in device_operations.py around lines ~864-866) or explicit obj.save() after set_librenms_device_id (as in librenms_api.py around lines ~261-262). This pattern prevents coupling mutation with persistence and preserves validation in between.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T19:15:13.104Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:249-267
Timestamp: 2026-03-09T19:15:13.104Z
Learning: In netbox_librenms_plugin/utils.py, ensure match_librenms_hardware_to_device_type returns None when DeviceTypeMapping.MultipleObjectsReturned is raised (fail-closed per inline comment). Callers must guard for result is None separately from the normal result check (e.g., if result is None: handle; elif result.get('matched'): ... ). Note that the success path uses match_type='mapping' (not 'exact'), distinguishing it from standard part_number/model exact lookups. Consider adding a unit test that asserts None is returned on MultipleObjectsReturned and that callers properly handle both None and dict results.
Applied to files:
netbox_librenms_plugin/utils.py
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html
[warning] 15-15: The type attribute must be present on elements.
(button-type-require)
[warning] 22-22: The type attribute must be present on
elements.(button-type-require)
🔇 Additional comments (17)
netbox_librenms_plugin/tests/test_librenms_api.py (1)
93-155: LGTM!netbox_librenms_plugin/views/sync/interfaces.py (1)
68-90: LGTM!Also applies to: 138-148
netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py (1)
166-185: LGTM!Also applies to: 339-381
netbox_librenms_plugin/tests/test_coverage_sync_view.py (1)
5-6: LGTM!Also applies to: 878-926
netbox_librenms_plugin/tables/interfaces.py (1)
55-57: LGTM!Also applies to: 183-207, 293-299, 368-392, 414-427, 435-438
netbox_librenms_plugin/tests/test_coverage_tables.py (1)
1448-1457: LGTM!Also applies to: 2663-2702
netbox_librenms_plugin/utils.py (1)
16-31: LGTM!netbox_librenms_plugin/tests/test_payload_hardening.py (1)
39-74: LGTM!Also applies to: 77-98, 101-134
netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
111-127: LGTM!Also applies to: 970-994
netbox_librenms_plugin/tests/test_sync_view_mismatch.py (1)
367-382: LGTM!Also applies to: 383-398
netbox_librenms_plugin/views/imports/actions.py (1)
219-240: LGTM!Also applies to: 242-260, 1513-1513
netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.py (1)
39-60: LGTM!Also applies to: 61-79, 80-104
netbox_librenms_plugin/import_utils/device_operations.py (1)
44-52: LGTM!netbox_librenms_plugin/tests/test_coverage_bulk_import.py (1)
132-139: LGTM!Also applies to: 2317-2347
netbox_librenms_plugin/tests/test_coverage_utils.py (1)
381-381: LGTM!Also applies to: 409-409, 426-427, 462-463
netbox_librenms_plugin/tests/test_import_utils.py (1)
1629-1630: LGTM!Also applies to: 1642-1643, 1659-1660
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
4103-4104: LGTM!Also applies to: 4173-4174, 4224-4225
Merged
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
commented
Jun 21, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/views/imports/actions.py (1)
604-611: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid synchronous LibreNMS fetches before background enqueue.
Line 609 calls
fetch_device_with_cache()for every selected device before the background-vs-sync decision. On a cold or expired import cache, this can hit the LibreNMS HTTP boundary once per selected device beforeImportDevicesJob.enqueue(), defeating the background path’s responsiveness. Build this map from already-cached entries only, and let the synchronous path/job fetch misses.♻️ Proposed fix
libre_devices_cache = {} for device_id in parsed_ids: - cached_device = fetch_device_with_cache(device_id, self.librenms_api) + cache_key = get_import_device_cache_key(device_id, self.librenms_api.server_key) + cached_device = cache.get(cache_key) if cached_device: libre_devices_cache[device_id] = cached_device🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/imports/actions.py` around lines 604 - 611, The pre-enqueue loop that calls fetch_device_with_cache() for every device in parsed_ids is causing synchronous HTTP requests to LibreNMS before the background-vs-sync decision, defeating background job responsiveness. Modify the loop to only populate libre_devices_cache from already-cached entries by accessing the cache directly without calling fetch_device_with_cache(), and remove any fetching logic from this pre-enqueue section. Let the synchronous import path and the background job handle cache misses by performing the actual fetches when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 604-611: The pre-enqueue loop that calls fetch_device_with_cache()
for every device in parsed_ids is causing synchronous HTTP requests to LibreNMS
before the background-vs-sync decision, defeating background job responsiveness.
Modify the loop to only populate libre_devices_cache from already-cached entries
by accessing the cache directly without calling fetch_device_with_cache(), and
remove any fetching logic from this pre-enqueue section. Let the synchronous
import path and the background job handle cache misses by performing the actual
fetches when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6a844f1-1f95-4143-992c-3f88946a0cfe
📒 Files selected for processing (16)
netbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.py
💤 Files with no reviewable changes (1)
- netbox_librenms_plugin/views/base/modules_view.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/imports/actions.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/interfaces_view.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (24)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-08T19:01:30.947Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/views/base/librenms_sync_view.py:36-41
Timestamp: 2026-03-08T19:01:30.947Z
Learning: In netbox_librenms_plugin/views/base/librenms_sync_view.py, when checking for a stored librenms_id, use explicit None-check instead of a bare truthiness test. Specifically, in get_context_data(), prefer: self.librenms_api.get_librenms_id(librenms_sync_device) is not None rather than a boolean conversion. This ensures a stored value of 0 is treated as present (since 0 is falsy in Python) and avoids misclassifying it as missing. Do not replace with bool(...) or a plain if self.librenms_api.get_librenms_id(...) check. Rationale: differentiates between None (no mapping) and valid numeric ids (including 0).
Applied to files:
netbox_librenms_plugin/views/base/librenms_sync_view.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T15:12:26.824Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/ip_addresses.py:94-103
Timestamp: 2026-06-01T15:12:26.824Z
Learning: For any redirect/tab URL building in netbox_librenms_plugin/views/sync, views/base, and views/object_sync, propagate the active multi-server `server_key` as a `?server_key=<key>` query parameter so users return to the same server’s tab after POST actions. When handling POST requests, read the POST-scoped `server_key` from `request.POST` and store it (e.g., `self._post_server_key`) with a fallback to `self.librenms_api.server_key`; use this POST-scoped key for both cache-key scoping and for constructing the redirect/tab URLs. Treat this as the intentional codebase-wide convention—do not flag the presence/usage of the `server_key` query parameter (or the corresponding POST-scoped `_post_server_key` pattern) in these views as an error.
Applied to files:
netbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/interfaces_view.py
📚 Learning: 2026-06-05T07:19:49.079Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/base/interfaces_view.py:158-165
Timestamp: 2026-06-05T07:19:49.079Z
Learning: When building OOB relationships from interface/device view code, call get_librenms_oob() using the resolved sync device (e.g., `lookup_device = get_librenms_sync_device(obj, server_key=...) or obj; oob = get_librenms_oob(lookup_device, ...)`) rather than calling get_librenms_oob(obj, ... ) directly. For VC members, OOB data (including shared-LOM markers) is stored on the resolved sync device, so resolving first is required to avoid dropping OOB rows.
Applied to files:
netbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/interfaces_view.py
📚 Learning: 2026-03-07T10:32:06.242Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:107-117
Timestamp: 2026-03-07T10:32:06.242Z
Learning: In netbox_librenms_plugin/utils.py, keep the Priority 1 loop to guard against None and bool values when accessing raw_cf.get(server_key). Do not replace the Priority 1 condition with a full get_librenms_device_id call. The two-pass design is intentional: Priority 1 performs quick sanity checks, while Priority 2 handles string normalization and full validation by calling get_librenms_device_id(member, server_key, auto_save=False).
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-07T22:38:43.110Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:552-584
Timestamp: 2026-03-07T22:38:43.110Z
Learning: In netbox_librenms_plugin/utils.py, do not propose replacing 'obj.custom_field_data.get("librenms_id") or {}' with a None check. The code intentionally uses 'or {}' to handle falsey values; downstream type guards treat them equivalently since LibreNMS IDs start at 1, making 0 equivalent to 'not set'. Do not modify this logic; keep the existing behavior for all falsey values.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T08:55:46.317Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:46.317Z
Learning: In netbox_librenms_plugin/utils.py, do not modify set_librenms_device_id to call obj.save(). It is mutator-only and should only update in-memory obj.custom_field_data[...] without persisting. Ensure callers perform persistence: after mutation, run full_clean() and then save() (as seen in device_operations.py around lines ~864-866) or explicit obj.save() after set_librenms_device_id (as in librenms_api.py around lines ~261-262). This pattern prevents coupling mutation with persistence and preserves validation in between.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T19:15:13.104Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:249-267
Timestamp: 2026-03-09T19:15:13.104Z
Learning: In netbox_librenms_plugin/utils.py, ensure match_librenms_hardware_to_device_type returns None when DeviceTypeMapping.MultipleObjectsReturned is raised (fail-closed per inline comment). Callers must guard for result is None separately from the normal result check (e.g., if result is None: handle; elif result.get('matched'): ... ). Note that the success path uses match_type='mapping' (not 'exact'), distinguishing it from standard part_number/model exact lookups. Consider adding a unit test that asserts None is returned on MultipleObjectsReturned and that callers properly handle both None and dict results.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
🪛 GitHub Actions: Test with all supported NetBox versions / 0_test-netbox (3.13).txt
netbox_librenms_plugin/tests/test_view_wiring.py
[warning] 1-1: PytestRemovedIn10Warning: Class-scoped fixture defined as instance method is deprecated. Instance attributes set in this fixture will NOT be visible to test methods (use @classmethod and set attributes on cls).
🪛 GitHub Actions: Test with all supported NetBox versions / 2_test-netbox (3.12).txt
netbox_librenms_plugin/tests/test_view_wiring.py
[warning] 1-1: PytestRemovedIn10Warning: Class-scoped fixture defined as instance method is deprecated. Instance attributes set in this fixture will NOT be visible to test methods.
🔇 Additional comments (30)
netbox_librenms_plugin/tests/test_modules_view.py (1)
3403-3430: LGTM!netbox_librenms_plugin/tests/test_reviewer_fixes.py (2)
198-201: LGTM!
267-268: LGTM!netbox_librenms_plugin/tests/test_view_wiring.py (3)
231-250: LGTM!
321-341: LGTM!
406-418: LGTM!netbox_librenms_plugin/utils.py (1)
1124-1153: LGTM!netbox_librenms_plugin/views/base/interfaces_view.py (2)
87-95: LGTM!
132-140: LGTM!netbox_librenms_plugin/views/base/librenms_sync_view.py (3)
15-15: LGTM!Also applies to: 116-116
191-210: LGTM!
263-266: LGTM!netbox_librenms_plugin/tests/test_coverage_base_views.py (1)
1001-1001: LGTM!Also applies to: 1049-1049, 1111-1111, 2127-2127, 2182-2182, 2233-2233
netbox_librenms_plugin/tables/interfaces.py (6)
55-60: LGTM!
193-210: LGTM!Also applies to: 296-302
371-395: LGTM!
417-441: LGTM!
465-482: LGTM!
185-191: 🔒 Security & PrivacyNo issue identified. The
get_missing_vlan_warning()function usesvidonly as a membership check and returns only static HTML (a hardcoded icon string or empty string). It does not interpolatevidor any untrusted data into the returned HTML, so usingmark_safe()is safe and appropriate.netbox_librenms_plugin/tests/test_coverage_tables.py (1)
1448-1456: LGTM!Also applies to: 2171-2228, 2701-2715
netbox_librenms_plugin/tests/test_coverage_utils.py (1)
801-835: LGTM!netbox_librenms_plugin/tables/cables.py (2)
1-2: LGTM!
122-158: LGTM!netbox_librenms_plugin/tables/modules.py (2)
84-117: LGTM!
828-833: LGTM!netbox_librenms_plugin/tests/test_enrich_remote_port_realdb.py (2)
107-142: LGTM!
144-180: LGTM!netbox_librenms_plugin/views/imports/actions.py (2)
39-39: LGTM!Also applies to: 173-242
665-693: LGTM!Also applies to: 755-755, 1156-1156, 1336-1358, 1443-1567, 1718-1823
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
5-6: LGTM!Also applies to: 859-869, 2329-2382, 4055-4241, 4258-4295
marcinpsk
commented
Jun 23, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Gate SingleCableVerifyView on dcim.view_device, validate and guard server_key before scoping the cache and ORM lookups, fail closed on malformed cached link/VLAN snapshots, render an empty VLAN table on an empty snapshot, and cache the VC member set instead of querying it per row.
…idation Fall back to the first usable server (skipping incomplete or malformed configs) rather than the first dict, normalize a legacy-mode server_key to default, cache get_device_info to drop a per-render LibreNMS call, unify regex-field validation behind validate_regex_field, and stop tracking .playwright-mcp scratch snapshots.
marcinpsk
force-pushed
the
fix/develop-hardening
branch
from
July 2, 2026 18:25
c6a626e to
0e570f6
Compare
…ctPermissionMixin helper Replace the inline `get_object_or_404(Model.objects.restrict(request.user, "view"), pk=…)` duplicated across the interface/module/vlan-group/cable/ip verify views with two mixin helpers on NetBoxObjectPermissionMixin (which already uses self.request.user and houses the object-permission machinery all 6 views share): - restricted_queryset(model, action="view") - restrict_object_or_404(model, action="view", **kwargs) This matches the repo's convention (shared logic lives on a mixin, e.g. _render_server_key / resolve_requested_server_key) instead of an inline snippet repeated 8×. Tests: convert the ip-verify server-key tests to real-DB device resolution; wire real superusers into the already-real-DB vlan-group verify tests; repoint the orthogonal enrichment / VC-resolution / post-handler mocks from the module-level get_object_or_404 patch to the restrict_object_or_404 seam. Full suite green (3344).
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…to real DB Security fix (new CodeRabbit finding on bonzo81#313): SaveVlanGroupOverridesView persisted VLAN overrides after a plain get_object_or_404(Device), gated only by plugin-wide write perm — so any plugin-writer could write overrides for a device they can't view. Add NetBoxObjectPermissionMixin to the view and route the lookup through restrict_object_or_404 so an out-of-scope pk 404s (red→green real-DB test). De-mock: replace every `restrict_object_or_404` test seam-stub with a real Device resolved through the real object-perm gate + restrict(), across the verify-view suites: - test_cable_verify: fully real Device/Interface/Cable + real cache (XSS escaping, stale-field recompute) - test_coverage_devices: real device/cache/table for interface + module + vlan-group + save-overrides (module test drives real has_perm from a precise ObjectPermission grant) - test_verify_views: real VC devices for the cable/interface VC-resolution tests - test_coverage_base_views2: real devices for the cable post-handler branch tests - test_reviewer_fixes / test_view_wiring: real VC device for the cable server-key threading tests Resolve LibreNMSSettings via apps.get_model in the two grant helpers so the autouse mock_librenms_config fixture (which patches the module attribute in the full suite) can't hand get_for_model() a MagicMock. Full suite green (3341).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/views/sync/modules.py (2)
1418-1426: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
InstallSelectedView's success/exception paths were not migrated to the HTMX partial swap.Early-exit validation paths (no selection, missing cache, invalid selection) now return
_render_modules_partial_after_action, but the success path (line 1426) and theValidationError/IntegrityErrorhandler (line 1420) still call_modules_redirect_response, unlikeInstallBranchView.post()which fully migrated all exit paths. Please confirm whether this is intentional (e.g., bulk multi-row selections deliberately keep the full-page reload) or an oversight in the HTMX rollout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/sync/modules.py` around lines 1418 - 1426, InstallSelectedView still mixes HTMX partial swaps with full-page redirects in its remaining exit paths. Update the success path and the ValidationError/IntegrityError handler in the post flow to use the same HTMX response helper as the other early exits, matching the migration pattern used by InstallBranchView.post() and keeping all InstallSelectedView outcomes consistent. Keep the existing reporting logic, but route the final response through the partial-swap path instead of _modules_redirect_response unless bulk selection is intentionally exempt.
1518-1554: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBroad
except Exceptioncan mask a successful bind behind a generic failure message.
_bind_interface_librenms_id(writes directly, no atomic wrapper) and_adopt_existing_template_interfaces(its own separatetransaction.atomic()block) are both wrapped in oneexcept Exception:. If the bind succeeds and commits, but the subsequent adopt call raises, the resulting message is a single generic "unexpected error while associating interface to installed module" — hiding that the port WAS actually bound.InstallModuleView.post()uses a narrower per-calltry/exceptaround just the bind call for this reason; this view should follow the same pattern so a bind success is still reported even if adoption subsequently fails.♻️ Proposed fix
- try: - bind_result = None - if bind_item and server_key: - bind_result = _bind_interface_librenms_id(target_device, bind_item, module.pk, server_key) - adopt_result = _adopt_existing_template_interfaces(target_device, module, server_key) + bind_result = None + if bind_item and server_key: + try: + bind_result = _bind_interface_librenms_id(target_device, bind_item, module.pk, server_key) + except Exception: + bind_result = { + "status": "failed", + "reason": "unexpected error while binding interface to installed module", + } + try: + adopt_result = _adopt_existing_template_interfaces(target_device, module, server_key) if adopt_result.get("status") == "bound": ... bind_result = adopt_result elif bind_result is None: bind_result = adopt_result except Exception: bind_result = { "status": "failed", - "reason": "unexpected error while associating interface to installed module", + "reason": "unexpected error while adopting standalone interfaces", }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/sync/modules.py` around lines 1518 - 1554, The combined broad except in the interface-association flow can hide a successful `_bind_interface_librenms_id` result behind a generic failure when `_adopt_existing_template_interfaces` raises afterward. Split the error handling in this view’s association logic the same way `InstallModuleView.post()` does: keep the bind call in its own try/except so a committed bind can still be reported, then handle adoption separately and only fall back to the generic failure message for the specific step that failed.
♻️ Duplicate comments (2)
netbox_librenms_plugin/views/sync/device_fields.py (1)
60-62: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated "live device info fetch" idiom (previously flagged).
The
get_device_info(id, use_cache=False)call plus its rationale comment is repeated verbatim acrossUpdateDeviceNameView,UpdateDeviceSerialView,UpdateDeviceTypeView,UpdateDevicePlatformView, andConvertLegacyLibreNMSIdView(and again inip_addresses.py'sget_management_ip). Still unresolved from the prior review pass — a sharedLibreNMSAPIMixin.get_live_device_info(librenms_id)helper would centralize this write-path caching rule.Also applies to: 136-138, 192-194, 262-264, 719-721
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/sync/device_fields.py` around lines 60 - 62, The live device info fetch pattern is duplicated across several views, so centralize it in a shared helper on LibreNMSAPIMixin, such as get_live_device_info(librenms_id), that always calls get_device_info(..., use_cache=False) with the write-path caching rationale. Update UpdateDeviceNameView, UpdateDeviceSerialView, UpdateDeviceTypeView, UpdateDevicePlatformView, ConvertLegacyLibreNMSIdView, and get_management_ip in ip_addresses.py to use the helper instead of repeating the same call and comment.netbox_librenms_plugin/views/sync/ip_addresses.py (1)
118-129: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated "live device info fetch" idiom.
Same
get_device_info(id, use_cache=False)pattern + rationale comment as indevice_fields.py's multiple views. A sharedLibreNMSAPIMixin.get_live_device_info(librenms_id)helper would centralize this write-path caching rule instead of repeating it here and in five other call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/sync/ip_addresses.py` around lines 118 - 129, Extract the repeated live-device lookup pattern into a shared helper on LibreNMSAPIMixin, such as get_live_device_info(librenms_id), and use it from the IP address sync flow instead of calling get_device_info(..., use_cache=False) directly. Move the existing success/type-checking behavior and rationale into that helper so the write-path caching rule is centralized, then update the current logic in the IP address resolver to call the shared method and keep the same fallback behavior when no valid IP is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html`:
- Around line 18-25: The Refresh control in bulk_import_confirm.html uses a
javascript: href, which will fail under stricter CSP; replace that anchor with a
real button-style control that triggers a reload without inline JavaScript.
Update the affected template markup around the warning message so the action
still refreshes the page, and keep the change localized to the bulk import
confirmation view.
In `@netbox_librenms_plugin/tests/test_cable_verify.py`:
- Around line 316-349: The String server_key test is not exercising the real
lookup path because TestServerKeyGuard._view mocks an unused instance attribute
instead of patching LibreNMSAPI.get_available_servers, and the current request
with device_id="" never proves the resolved key was used. Update the test setup
to patch the classmethod imported by SingleCableVerifyView.post, and change
test_valid_string_server_key_is_still_honoured to post a real device_id and
assert the resulting cache lookup or sync path uses the "prod" server_key so the
regression is actually verified.
In `@netbox_librenms_plugin/tests/test_coverage_base_views2.py`:
- Around line 690-720: The test is relying on a dead instance mock for
server-key resolution, so it does not actually verify the fallback path in
SingleCableVerifyView.post(). Patch LibreNMSAPI.get_available_servers as the
classmethod used by the view, and keep the assertion on get_cache_key so the
test proves an unconfigured posted server_key falls back to the session/active
server rather than using the raw request value.
In `@netbox_librenms_plugin/tests/test_coverage_tables.py`:
- Around line 2701-2715: The current XSS coverage in
test_render_vlans_escapes_malicious_vid only exercises the untagged_vlan path,
so the missing-vlan warning branch in render_vlans/get_missing_vlan_warning is
still untested. Extend this test in test_render_vlans_escapes_malicious_vid (or
add a sibling case nearby) with a non-empty missing_vlans value containing the
malicious VID so the warning HTML path is rendered too, and assert the dangerous
markup is escaped there as well. Use the existing _make_interface_table helper
and the render_vlans method to verify both inline summary and warning tooltip
output remain safe.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 739-743: In InstallModuleView.post(), the occupied-bay early
return is using the wrong device for the HTMX refresh. Update the warning branch
that checks locked_bay.installed_module to pass page_device into
_render_modules_partial_after_action, matching the other render paths in this
method. This keeps the returned panel aligned with the page device in VC member
scenarios and avoids swapping in the target_device panel.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 1418-1426: InstallSelectedView still mixes HTMX partial swaps with
full-page redirects in its remaining exit paths. Update the success path and the
ValidationError/IntegrityError handler in the post flow to use the same HTMX
response helper as the other early exits, matching the migration pattern used by
InstallBranchView.post() and keeping all InstallSelectedView outcomes
consistent. Keep the existing reporting logic, but route the final response
through the partial-swap path instead of _modules_redirect_response unless bulk
selection is intentionally exempt.
- Around line 1518-1554: The combined broad except in the interface-association
flow can hide a successful `_bind_interface_librenms_id` result behind a generic
failure when `_adopt_existing_template_interfaces` raises afterward. Split the
error handling in this view’s association logic the same way
`InstallModuleView.post()` does: keep the bind call in its own try/except so a
committed bind can still be reported, then handle adoption separately and only
fall back to the generic failure message for the specific step that failed.
---
Duplicate comments:
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 60-62: The live device info fetch pattern is duplicated across
several views, so centralize it in a shared helper on LibreNMSAPIMixin, such as
get_live_device_info(librenms_id), that always calls get_device_info(...,
use_cache=False) with the write-path caching rationale. Update
UpdateDeviceNameView, UpdateDeviceSerialView, UpdateDeviceTypeView,
UpdateDevicePlatformView, ConvertLegacyLibreNMSIdView, and get_management_ip in
ip_addresses.py to use the helper instead of repeating the same call and
comment.
In `@netbox_librenms_plugin/views/sync/ip_addresses.py`:
- Around line 118-129: Extract the repeated live-device lookup pattern into a
shared helper on LibreNMSAPIMixin, such as get_live_device_info(librenms_id),
and use it from the IP address sync flow instead of calling get_device_info(...,
use_cache=False) directly. Move the existing success/type-checking behavior and
rationale into that helper so the write-path caching rule is centralized, then
update the current logic in the IP address resolver to call the shared method
and keep the same fallback behavior when no valid IP is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 108c41ef-282b-4b4d-9c93-3f004e52a7ce
📒 Files selected for processing (75)
.gitignorenetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/module_mismatch_modal.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_api.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_enrich_remote_port_realdb.pynetbox_librenms_plugin/tests/test_id_lookup_lossy_coercion_realdb.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_interface_sync_button_type.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_module_interface_bind_message.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_payload_hardening.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_render_server_key_degrade.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vlan_sync_group_missing.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/vlans.py
💤 Files with no reviewable changes (1)
- netbox_librenms_plugin/tests/test_sync_devices.py
…rom adopt failures Three CodeRabbit findings on the module-sync views: - InstallModuleView bay-occupied exit rendered `target_device`'s panel; for a VC-member row action (target_device != page_device) that swaps in the member's panel and the page's own bays vanish. Swap `page_device` like every other exit path. - InstallSelectedView's success + ValidationError/IntegrityError paths still returned the full-page HX-Redirect while its four early exits (and all of InstallBranchView) return the in-place HTMX partial — mixing swap modes flips this row action's behaviour. Return the partial on every exit. - UpdateModuleInterfaceView wrapped the port-bind AND the adopt step in one broad `except Exception`; _bind_interface_librenms_id writes with no atomic, so a bind that committed was masked behind a generic "associating" failure when the later adopt raised. Isolate the bind in its own try/except (mirroring InstallModuleView) and, on adopt failure, warn without overwriting a committed bind. Tests (real device/module where the swap path renders; the two exit-path assertions drive the InstallSelected/UpdateModuleInterface post flows): red→green verified.
…ibreNMSAPIMixin The `get_device_info(id, use_cache=False)` idiom — persist LIVE LibreNMS values on a write path, not the possibly-stale sync-tab render snapshot — was copy-pasted with its rationale across six write views. Add LibreNMSAPIMixin.get_live_device_info(librenms_id), which every one of those views already inherits (they use self.librenms_api), and route all six through it: name/serial/type/platform update, legacy-id convert, and the OOB Primary-IP resolution. The two sites that keep a site-specific note (convert serial gate, Primary-IP decision) now reference the helper instead of a now-hidden use_cache flag. Helper test uses a recording fake to prove the cache is bypassed; existing view coverage exercises behaviour unchanged.
…LAN XSS escaping - bulk_import_confirm.html's partial-cache "Refresh" control was an `<a href="javascript:...">` pseudo-protocol link (CSP-hostile, flagged accessibility/security). Replace with a real `<button type="button" onclick="window.location.reload()">`; assert no `javascript:` in the render. - Add the missing render_vlans coverage case with a non-empty `missing_vlans` carrying an XSS vid, so the escaping guarantee is exercised on that branch too (the existing case only covered summary/tooltip).
…queryset mock - Restore three cable-verify classes an earlier full-file rewrite dropped (TestServerKeyGuard, broken-default-server render/verify degrade). TestServerKeyGuard's honoured-key test previously posted device_id="" (which skips the code path that consumes server_key) and stubbed the wrong seam (self._librenms_api.get_available_servers), so it passed regardless of the fix. Rewrite it to use a real device and patch the LibreNMSAPI.get_available_servers classmethod the guard actually consults, asserting the resolved server_key == the posted configured key. - Extract the slice-aware `__getitem__` queryset mock (repeated verbatim in 12 device_filter closures, supporting validate_device_for_import's [:2] unique-serial guard) into one _matchable_filter_result helper so the slice contract can't drift out of sync.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/views/sync/modules.py (1)
718-719: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate nonblank posted
server_keybefore using it for binding/cache scope.These changed fallbacks only handle blank values; a forged nonblank key is still honored and can scope cache reads or LibreNMS custom-field bindings under an unconfigured namespace. Use the same configured-key guard before falling back to the active server.
Proposed fix
- server_key = (request.POST.get("server_key") or "").strip() or self.librenms_api.server_key + requested_server_key = (request.POST.get("server_key") or "").strip() + server_key = self.resolve_requested_server_key({"server_key": requested_server_key})Apply the same replacement in both changed POST paths.
Based on learnings, invalid posted
server_keyvalues should fall back to the currently configured default/active server key rather than being honored when unconfigured.Also applies to: 1506-1512
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/sync/modules.py` around lines 718 - 719, The binding/cache lookup is still trusting any nonblank posted server_key, which can scope operations under an unconfigured namespace. In the POST handling around _resolve_single_install_binding_item and the other affected path, validate the submitted server_key against the same configured-key guard used for the active server, and if it is missing or invalid, fall back to self.librenms_api.server_key instead of honoring the request value. Apply this fix consistently in both POST locations so cache reads and LibreNMS custom-field bindings always use the configured default/active server key.Source: Learnings
netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
543-543: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
vrf_idbefore the VRF lookup.
A non-numeric JSON value will blow up inIPAddress.objects.filter(..., vrf__id=vrf_id), and the broadexcept Exceptionturns that into a 500. Coerce tointor return a 400 before calling_find_existing_ip().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/base/ip_addresses_view.py` at line 543, The vrf_id handling in IPAddressCreateView should validate and coerce the incoming JSON value before it reaches _find_existing_ip(). Add a numeric check around data.get("vrf_id") in the request flow so non-integer values are rejected early with a 400 response instead of causing IPAddress.objects.filter(..., vrf__id=vrf_id) to fail. Keep the fix localized to the vrf_id parsing/validation path and the _find_existing_ip() lookup call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Line 543: The vrf_id handling in IPAddressCreateView should validate and
coerce the incoming JSON value before it reaches _find_existing_ip(). Add a
numeric check around data.get("vrf_id") in the request flow so non-integer
values are rejected early with a 400 response instead of causing
IPAddress.objects.filter(..., vrf__id=vrf_id) to fail. Keep the fix localized to
the vrf_id parsing/validation path and the _find_existing_ip() lookup call site.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 718-719: The binding/cache lookup is still trusting any nonblank
posted server_key, which can scope operations under an unconfigured namespace.
In the POST handling around _resolve_single_install_binding_item and the other
affected path, validate the submitted server_key against the same configured-key
guard used for the active server, and if it is missing or invalid, fall back to
self.librenms_api.server_key instead of honoring the request value. Apply this
fix consistently in both POST locations so cache reads and LibreNMS custom-field
bindings always use the configured default/active server key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b12b3f71-fd0a-432a-b891-705f977cf6cc
📒 Files selected for processing (20)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.htmlnetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/modules.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (8)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments live in
templates/netbox_librenms_plugin/htmx/including:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/modules.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.py
**/views/object_sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Object sync view methods must create instances of concrete table views, copy the
requestobject, and callget_context_data(). VMs must skip cables and VLANs by returningNonefrom thoseget_*_context()methods.
Files:
netbox_librenms_plugin/views/object_sync/devices.py
🧠 Learnings (32)
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django template files under netbox_librenms_plugin/templates/**/*.html, do NOT flag template expressions like accessing a chained attribute on a possibly-None variable (e.g., `librenms_sync_device.pk` when `librenms_sync_device` may be None) as a NullPointerError/AttributeError. Django’s template attribute lookup resolves failed lookups to `TEMPLATE_STRING_IF_INVALID` (empty string by default), so comparisons such as `object.pk == librenms_sync_device.pk` will evaluate against `''` and safely result in False rather than raising a template error.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
📚 Learning: 2026-05-25T21:48:19.264Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html:12-12
Timestamp: 2026-05-25T21:48:19.264Z
Learning: In this plugin’s HTMX form templates, `hx-include` selectors that target toggle preference wrapper `<span>` element IDs (e.g., `#use-sysname-toggle`, `#strip-domain-toggle`, `#auto-create-ipam-toggle`) are intentional. Those wrapper spans contain both the hidden `off` fallback input and the checkbox; HTMX must include the wrapper so the correct value is serialized, including the unchecked/off state. Do not recommend changing `hx-include` to the checkbox IDs with the `-cb` suffix, since it would omit the hidden fallback and break unchecked/off state submission.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
📚 Learning: 2026-06-14T22:58:16.581Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html:196-196
Timestamp: 2026-06-14T22:58:16.581Z
Learning: In Django templates, the `{% if %}` tag does not support parenthetical grouping. Do not suggest adding parentheses like `{% if (not x) %}` or `{% if (a or b) %}`—these can raise `TemplateSyntaxError` (e.g., “Could not parse the remainder”). Instead, express the logic using Django template operator precedence rules (not binds tighter than and, and binds tighter than or) and refactor (e.g., via separate conditions/`{% if %}` blocks) when precedence alone can’t express the intended grouping.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-07-01T16:41:50.451Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tables/cables.py:62-76
Timestamp: 2026-07-01T16:41:50.451Z
Learning: When rendering Bootstrap/Tabler badges in this repo (netbox-librenms-plugin), always pair a solid background utility `bg-*` with an explicit text utility that provides appropriate contrast (e.g., `bg-danger` + `text-white`, `bg-warning` + `text-dark`, `bg-purple` + `text-white`). Follow the existing badge contrast convention enforced by `netbox_librenms_plugin/tests/test_badge_contrast.py`. Do not suggest Tabler’s semantic `text-*-fg` token classes (e.g., `text-purple-fg`), since they are not used in this codebase and are not covered by the contrast test.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-07-02T13:36:15.226Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/interfaces_view.py:226-226
Timestamp: 2026-07-02T13:36:15.226Z
Learning: When reviewing netbox-librenms-plugin view code that handles POSTed `server_key`, treat the plugin-wide convention as intentional: if the posted `server_key` is not present in `LibreNMSAPI.get_available_servers()`, the request should fall back to the currently configured default/active server key (not reject/fail-closed and not treat it as an error for that single view). Do not flag individual instances of this fallback pattern as incorrect “invalid server_key” validation. Any change to fail-closed behavior must be a coordinated cross-cutting change applied uniformly across all affected sites/views, not a one-view patch.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-06-01T15:12:26.824Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/ip_addresses.py:94-103
Timestamp: 2026-06-01T15:12:26.824Z
Learning: For any redirect/tab URL building in netbox_librenms_plugin/views/sync, views/base, and views/object_sync, propagate the active multi-server `server_key` as a `?server_key=<key>` query parameter so users return to the same server’s tab after POST actions. When handling POST requests, read the POST-scoped `server_key` from `request.POST` and store it (e.g., `self._post_server_key`) with a fallback to `self.librenms_api.server_key`; use this POST-scoped key for both cache-key scoping and for constructing the redirect/tab URLs. Treat this as the intentional codebase-wide convention—do not flag the presence/usage of the `server_key` query parameter (or the corresponding POST-scoped `_post_server_key` pattern) in these views as an error.
Applied to files:
netbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-06-25T07:07:59.192Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 114
File: netbox_librenms_plugin/tests/test_oob_sync_review_fixes.py:0-0
Timestamp: 2026-06-25T07:07:59.192Z
Learning: When writing/adjusting tests (and any review/test helper code) that interact with NetBox’s custom User model, do not assume the user model has an `is_staff` field. If you need to check or set user privileges/eligibility, use fields that are known to exist on the NetBox User model (e.g., `is_superuser` and `is_active`) instead. Avoid setting `is_staff` (it can raise Django `FieldError` if that field doesn’t exist on the custom model).
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_mixins_helpers.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-03-07T09:14:06.791Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:324-331
Timestamp: 2026-03-07T09:14:06.791Z
Learning: In netbox_librenms_plugin/views/base/cables_view.py, do not treat cache.ttl() usage as a portability issue. NetBox requires Redis as the cache backend (since NetBox v2.6), so django-redis cache.ttl() and cache.pttl() extensions are available. Consider this as a project-specific guideline: cache.ttl() is intentional/safe in this codebase.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-06-05T07:19:49.079Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/base/interfaces_view.py:158-165
Timestamp: 2026-06-05T07:19:49.079Z
Learning: When building OOB relationships from interface/device view code, call get_librenms_oob() using the resolved sync device (e.g., `lookup_device = get_librenms_sync_device(obj, server_key=...) or obj; oob = get_librenms_oob(lookup_device, ...)`) rather than calling get_librenms_oob(obj, ... ) directly. For VC members, OOB data (including shared-LOM markers) is stored on the resolved sync device, so resolving first is required to avoid dropping OOB rows.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.py
📚 Learning: 2026-06-26T09:04:49.793Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_verify_views.py:281-343
Timestamp: 2026-06-26T09:04:49.793Z
Learning: When implementing per-object permission resolution in verify views (e.g., like `SingleIPAddressVerifyView`), `_required_perms_for_object` should: (1) if `object_type` is explicit, gate on the exact model permission for that target type; (2) if `object_type` is not explicit, resolve the object id to its model without reading the object’s data (avoid fetching the object just to determine permissions); and (3) in ambiguous cases, fail closed by requiring all relevant view permissions (deny unless both applicable permissions are satisfied). Add/extend DB-backed tests to cover allow/deny paths and the “no `object_type`” case.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.py
📚 Learning: 2026-03-08T08:57:43.392Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/object_sync/devices.py:83-100
Timestamp: 2026-03-08T08:57:43.392Z
Learning: In views under netbox_librenms_plugin/views/object_sync, server_key values come from admin-controlled PLUGINS_CONFIG dict keys (e.g., "default", "production") and are not user input. Therefore URL-encoding them via urlencode() is unnecessary defensiveness. Do not flag direct string interpolation of server_key into query strings as a URL-injection or encoding issue. This guidance should apply to similar views in the same directory.
Applied to files:
netbox_librenms_plugin/views/object_sync/devices.py
📚 Learning: 2026-05-05T09:58:50.179Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/object_sync/devices.py:57-75
Timestamp: 2026-05-05T09:58:50.179Z
Learning: In object_sync view classes that pass Django/NetBox `request` into child table context helpers (e.g., for interfaces/cables/IPs/vlans/modules), ensure the child view stores `copy.copy(request)` rather than the original `request` object. Apply this consistently across similar sync views (such as the pattern used in `VMLibreNMSSyncView` in `vms.py`) to prevent cross-view request mutation when the child view modifies the request.
Applied to files:
netbox_librenms_plugin/views/object_sync/devices.py
📚 Learning: 2026-06-25T07:14:19.587Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/modules_view.py:1073-1074
Timestamp: 2026-06-25T07:14:19.587Z
Learning: In netbox_librenms_plugin/views/base/modules_view.py and netbox_librenms_plugin/views/sync/modules.py, treat LibreNMS `entPhysicalIndex` as an end-to-end integer invariant (it originates from an LibreNMS int DB column and is preserved as an int through the module inventory/sync pipeline). When reviewing code, do not flag mixed string/int `entPhysicalIndex` handling or request additional `int()` normalization solely as a defensive measure against string indices. Only recommend `int()` conversion/normalization if there is concrete evidence in the code path that values are actually being converted to strings (e.g., explicit casts, JSON serialization/deserialization steps that coerce to strings, or external inputs known to provide strings).
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
🪛 ast-grep (0.44.0)
netbox_librenms_plugin/tests/test_reviewer_fixes.py
[info] 287-287: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netbox_librenms_plugin/tests/test_view_wiring.py
[info] 455-455: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netbox_librenms_plugin/tests/test_cable_verify.py
[info] 57-57: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 234-234: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netbox_librenms_plugin/tests/test_coverage_devices.py
[info] 564-564: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"device_id": device.pk, "ent_physical_index": 10, "server_key": "default"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netbox_librenms_plugin/tests/test_coverage_base_views2.py
[info] 727-727: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"device_id": device.pk, "local_port_id": 10, "server_key": "ghost"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netbox_librenms_plugin/tests/test_verify_views.py
[info] 51-51: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 797-797: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"device_id": device_pk, "vid_group_map": {"10": 3}, "server_key": self.SERVER_KEY})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (20)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html (1)
18-26: LGTM!netbox_librenms_plugin/views/sync/ip_addresses.py (1)
107-131: LGTM!netbox_librenms_plugin/tests/test_mixins_helpers.py (1)
90-113: LGTM!netbox_librenms_plugin/tests/test_reviewer_fixes.py (1)
13-14: LGTM!Also applies to: 200-203, 258-351, 399-447
netbox_librenms_plugin/views/sync/device_fields.py (1)
60-60: LGTM!Also applies to: 134-134, 188-188, 256-256, 703-716, 741-744
netbox_librenms_plugin/tests/test_coverage_tables.py (1)
1448-1457: LGTM!Also applies to: 2171-2185, 2194-2198, 2209-2228, 2673-2731
netbox_librenms_plugin/tests/test_import_utils.py (1)
10-27: LGTM!Also applies to: 1643-1692, 1748-1849, 1898-1996, 2028-2083, 2111-2190, 2272-2275, 2615-2649, 3687-3696, 4902-4909, 5006-5013, 6094-6142
netbox_librenms_plugin/tests/test_ip_verify.py (1)
77-140: LGTM!Also applies to: 146-154, 160-163
netbox_librenms_plugin/views/mixins.py (1)
7-7: LGTM!Also applies to: 247-281, 349-389, 391-406
netbox_librenms_plugin/views/base/cables_view.py (1)
164-167: LGTM!Also applies to: 359-431, 469-662
netbox_librenms_plugin/tests/test_cable_verify.py (1)
4-78: LGTM!Also applies to: 81-161, 164-222, 225-283, 290-333, 337-382
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
859-869: LGTM!Also applies to: 1132-1161, 2272-2330, 2332-2385, 4056-4092, 4353-4390, 4617-4672
netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
44-68: LGTM!Also applies to: 717-748, 750-829, 852-917, 919-996, 1944-2023, 2046-2130
netbox_librenms_plugin/tests/test_verify_views.py (1)
14-16: LGTM!Also applies to: 26-57, 74-124, 136-153, 180-257, 414-465, 524-540, 559-575, 639-664, 705-738, 753-847
netbox_librenms_plugin/tests/test_view_wiring.py (1)
210-218: LGTM!Also applies to: 241-261, 343-363, 425-519
netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
172-184: LGTM!Also applies to: 265-270, 361-440, 459-479, 522-542, 544-609, 612-638
netbox_librenms_plugin/views/object_sync/devices.py (1)
123-190: LGTM!Also applies to: 193-359, 362-497, 512-512, 558-615
netbox_librenms_plugin/views/sync/modules.py (1)
650-683: LGTM!Also applies to: 686-692, 695-717, 720-805, 811-929, 1287-1431, 1488-1505, 1513-1611, 1626-1728, 1789-1982
netbox_librenms_plugin/tests/test_sync_modules.py (1)
17-25: LGTM!Also applies to: 151-165, 182-193, 267-272, 1182-1222, 1472-2038, 3122-3337, 6481-6520, 6820-6826
netbox_librenms_plugin/tests/test_coverage_devices.py (1)
19-80: LGTM!Also applies to: 299-479, 482-627, 630-796, 807-816, 911-1064
marcinpsk
commented
Jul 2, 2026
SingleIPAddressVerifyView.post() read vrf_id straight from the JSON body and passed it into
IPAddress.objects.filter(vrf__id=…) inside _find_existing_ip. A non-numeric (or boolean) vrf_id
raises ValueError there, which the method's broad `except Exception` turns into an opaque 500. Any
caller past the object-view gate could trigger it with {"vrf_id": "abc"}.
Coerce/validate vrf_id to int up front and fail closed with a 400 (None — no VRF — still passes
through). Same "fail closed on malformed payloads" hardening already applied to the JSON body and the
server_key. Real-DB e2e test creates an IP at the posted address (so the vrf__id filter is actually
reached) and asserts 400 for non-numeric/list/bool vrf_id and 200 for a numeric string.
…efore binding
The module install/replace/bind POST flows resolved server_key as
`(request.POST.get("server_key") or "").strip() or self.librenms_api.server_key` — handling only a
BLANK key. A forged non-blank key that names no configured server was honoured and used to scope
cache reads and to write the LibreNMS custom-field binding
(custom_field_data["librenms_id"][server_key]) under an unconfigured namespace. Every other
server-key entry point on this branch already validates against LibreNMSAPI.get_available_servers()
(the IP-verify view, the sync/cable views via resolve_requested_server_key) — the module views were
the lone gap.
Add LibreNMSAPIMixin.resolve_posted_server_key(data): honour only a configured string key, else fall
back to the ACTIVE client server (so the port-bind still runs — unlike resolve_requested_server_key,
which degrades to None for cached GET renders). Route all six module server_key resolutions
(InstallModule/InstallBranch/InstallSelected/UpdateModuleInterface/ReplaceModule POST +
ModuleMismatchPreview GET) through it. Helper test covers configured/forged/blank/missing (red→green);
the affected view tests now configure their posted "production"/"prod" server (mirrors a real
multi-server deployment).
…plicates A prior partial move (b15fe53) scattered copies of these tests into module-specific files but left test_reviewer_fixes.py in place with the originals — so several classes existed in two files at once (CodeRabbit flagged TestSingleCableVerifyServerKey as a verbatim duplicate). Finish the job so each of the 8 classes lives in exactly one home: - normalize-mapping + create-platform-full-clean → test_coverage_device_fields - generate/load VC member name → test_virtual_chassis - all-server-mappings validation → test_coverage_sync_view (beside TestBuildAllServerMappings) - render-device-selection escape → test_coverage_tables - Drop the two verbatim duplicates (SingleCableVerifyServerKey, ImportSingleDeviceLazyValidation) — canonical copies already live in test_view_wiring / test_import_utils; also remove the misplaced copies the prior move had dumped into test_view_wiring. - Delete test_reviewer_fixes.py. De-mock where the real object drives the behaviour: the render-escape test builds a real virtual chassis + a real member Device with an XSS name; the VC-member-name loader reads a real LibreNMSSettings row (resolved via apps.get_model and restored over the session-wide LibreNMSSettings mock for the loader's deferred import). None/boolean/DB-error cases keep a mock — a NOT-NULL CharField cannot hold those values. Full suite green (3337).
marcinpsk
commented
Jul 3, 2026
marcinpsk
commented
Jul 6, 2026
marcinpsk
commented
Jul 11, 2026
renormalize_device_type_mappings called apply_normalization_rules(scope= "device_type") once per DeviceTypeMapping row without preloaded_rules, so it re-queried NormalizationRule on every row — an avoidable N+1 that scales with the number of existing mappings during upgrade. Preload the device_type rule chain once via preload_normalization_rules() and thread the dict into every per-row apply_normalization_rules() call, so the whole data migration issues a constant number of rule queries. Behaviour is unchanged; each row is still re-keyed exactly as before. Test: test_rule_queries_are_constant_regardless_of_row_count runs the migration over multiple mappings inside CaptureQueriesContext and asserts the NormalizationRule query count does not scale with row count (was 1/row).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py`:
- Around line 25-36: Move the preload_normalization_rules(scope="device_type")
call inside the existing try block in the migration, so import and preload/query
failures are both handled by the existing except path; keep preloaded_rules
available to the subsequent per-row normalization logic when preload succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5b5fe8f-ff9b-4cc0-b7cd-83eb975e247a
📒 Files selected for processing (2)
netbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.pynetbox_librenms_plugin/tests/test_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
🧠 Learnings (15)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-07-01T16:41:50.451Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tables/cables.py:62-76
Timestamp: 2026-07-01T16:41:50.451Z
Learning: When rendering Bootstrap/Tabler badges in this repo (netbox-librenms-plugin), always pair a solid background utility `bg-*` with an explicit text utility that provides appropriate contrast (e.g., `bg-danger` + `text-white`, `bg-warning` + `text-dark`, `bg-purple` + `text-white`). Follow the existing badge contrast convention enforced by `netbox_librenms_plugin/tests/test_badge_contrast.py`. Do not suggest Tabler’s semantic `text-*-fg` token classes (e.g., `text-purple-fg`), since they are not used in this codebase and are not covered by the contrast test.
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-25T07:07:59.192Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 114
File: netbox_librenms_plugin/tests/test_oob_sync_review_fixes.py:0-0
Timestamp: 2026-06-25T07:07:59.192Z
Learning: When writing/adjusting tests (and any review/test helper code) that interact with NetBox’s custom User model, do not assume the user model has an `is_staff` field. If you need to check or set user privileges/eligibility, use fields that are known to exist on the NetBox User model (e.g., `is_superuser` and `is_active`) instead. Avoid setting `is_staff` (it can raise Django `FieldError` if that field doesn’t exist on the custom model).
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-07-02T21:46:46.384Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_coverage_bulk_import.py:132-140
Timestamp: 2026-07-02T21:46:46.384Z
Learning: In netbox_librenms_plugin/tests, keep the `_stub_norm_preload` autouse fixture (which patches `netbox_librenms_plugin.import_utils.bulk_import.preload_normalization_rules` to return `{}`) intentionally duplicated per mock-based test class/module. Do NOT hoist it into a global `conftest.py` autouse fixture, because that would apply the patch repo-wide and mask the real `preload_normalization_rules` behavior in real-DB tests that are meant to exercise it (see issue `#90`). If you need this patch, scope it to the specific mock-based tests that should stub normalization rules.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
🔇 Additional comments (2)
netbox_librenms_plugin/tests/test_utils.py (1)
1242-1276: LGTM!netbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py (1)
48-54: LGTM! Thepreloaded_rulesdict is correctly threaded intoapply_normalization_rules, matching its documentedpreloaded_rulescontract (unscoped lookup, no manufacturer), and the per-row savepoint/try-except from the prior review round is preserved.
The 0011 renormalize migration preloads the device_type NormalizationRule chain once (the N+1 fix) before the per-row loop. That preload issues a DB query outside any savepoint, so a failure there aborted the whole migration before the per-row guards ran — and, being a DB error, poisoned the outer atomic transaction so catching it alone could not leave a clean state. Wrap the single preload in its own transaction.atomic() savepoint and bail out on failure, logging and leaving every row untouched, matching the per-row savepoint pattern and the migration's "leave rows untouched" contract. The single-preload N+1 fix is preserved. Real-DB red->green test drives a genuine preload DB error (SELECT 1/0).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py`:
- Around line 44-45: Replace the runtime preload_normalization_rules call in the
0011 migration with migration-local logic that obtains the historical
NormalizationRule via apps.get_model(). Reimplement the required device_type
normalization lookup within the migration, preserving the existing re-keying
behavior without importing netbox_librenms_plugin.utils or live models.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c998b4d7-631b-4b77-9773-1c590051236d
📒 Files selected for processing (2)
netbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.pynetbox_librenms_plugin/tests/test_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
🧠 Learnings (15)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-07-01T16:41:50.451Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tables/cables.py:62-76
Timestamp: 2026-07-01T16:41:50.451Z
Learning: When rendering Bootstrap/Tabler badges in this repo (netbox-librenms-plugin), always pair a solid background utility `bg-*` with an explicit text utility that provides appropriate contrast (e.g., `bg-danger` + `text-white`, `bg-warning` + `text-dark`, `bg-purple` + `text-white`). Follow the existing badge contrast convention enforced by `netbox_librenms_plugin/tests/test_badge_contrast.py`. Do not suggest Tabler’s semantic `text-*-fg` token classes (e.g., `text-purple-fg`), since they are not used in this codebase and are not covered by the contrast test.
Applied to files:
netbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/migrations/0011_renormalize_device_type_mappings.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-06-25T07:07:59.192Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 114
File: netbox_librenms_plugin/tests/test_oob_sync_review_fixes.py:0-0
Timestamp: 2026-06-25T07:07:59.192Z
Learning: When writing/adjusting tests (and any review/test helper code) that interact with NetBox’s custom User model, do not assume the user model has an `is_staff` field. If you need to check or set user privileges/eligibility, use fields that are known to exist on the NetBox User model (e.g., `is_superuser` and `is_active`) instead. Avoid setting `is_staff` (it can raise Django `FieldError` if that field doesn’t exist on the custom model).
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-07-02T21:46:46.384Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_coverage_bulk_import.py:132-140
Timestamp: 2026-07-02T21:46:46.384Z
Learning: In netbox_librenms_plugin/tests, keep the `_stub_norm_preload` autouse fixture (which patches `netbox_librenms_plugin.import_utils.bulk_import.preload_normalization_rules` to return `{}`) intentionally duplicated per mock-based test class/module. Do NOT hoist it into a global `conftest.py` autouse fixture, because that would apply the patch repo-wide and mask the real `preload_normalization_rules` behavior in real-DB tests that are meant to exercise it (see issue `#90`). If you need this patch, scope it to the specific mock-based tests that should stub normalization rules.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_utils.py
🔇 Additional comments (1)
netbox_librenms_plugin/tests/test_utils.py (1)
1364-1408: LGTM!
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Develop-targeted hardening pass on develop-owned code. Each fix is paired with a red→green test.
format_htmlinstead ofmark_safe.server_keyonly when it names a configured server, else fall back to the active server; normalise legacy mode todefault.int(); reject float / non-int / blank / non-positive ids.device_typenormalization scope when matching hardware, threading preloaded rules to avoid a per-device N+1.aria-labelon NetBox-only modal checkboxes; correct button semantics; escaped redirect targets.Motivation / Problem
Bug / maintenance. Keep develop-owned correctness and security fixes off the feature stack.
Scope of Change
How Was This Tested?
Risk Assessment
Hardening only. Behaviour change is fail-closed on malformed or forged input; no feature changes, so existing users are unaffected and no unintended imports/updates are introduced.
Backwards Compatibility
Summary by CodeRabbit