feat: data shapes - #118
Open
marcinpsk wants to merge 90 commits into
Open
Conversation
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds a LibreNMS data-shape workflow. It captures, compresses, anonymizes, fingerprints, validates, and replays API recordings. It adds NetBox UI access, issue submission, fixtures, manifests, tests, and contributor documentation. ChangesData-shape workflow
Integration and validation
Contribution support
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to The PR adds capture/replay tooling and bundled data-shape fixtures, but the current head still has a replay-path null-handling bug that can trigger database access, plus sync correctness issues where cache-only requests may go live and cable verification may update the wrong row or fail. These risks should be fixed or explicitly accepted before merging. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description includes the required summary, motivation, scope, testing, risk, and backward-compatibility sections. It provides relevant implementation and testing details. The optional Other Notes section is omitted, and manual test steps are not listed despite manual testing being reported. Full details: Docstring CoverageExplanation Docstring coverage is 80.76% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 582 functions across 48 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
marcinpsk
force-pushed
the
feat/data-shapes
branch
4 times, most recently
from
June 21, 2026 08:39
e017fb7 to
382c407
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@docs/development/contributing-data-shapes.md`:
- Around line 49-64: The markdown document has spacing and formatting issues
with code fences that trigger linting warnings. Add blank lines before and after
each code fence block (the json block starting at line 49 and the two console
blocks at lines 56 and 61), and remove the `$` prompt characters from the
beginning of each command line in the console code blocks, leaving just the
command text itself. This ensures proper markdown linting compliance for both
blank-line spacing around fences (MD031) and console command formatting (MD014).
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 688-697: The device matching logic in this code block only checks
devices linked via IPAddress.assigned_object.device but misses devices linked
through Device.oob_ip. After the existing IPAddress lookup (where you get the
device from assigned_object), add a fallback check to also query for devices
where the oob_ip field matches the primary_ip value. This ensures the
refresh-time matching is consistent with the validate_device_for_import() method
and prevents duplicate imports from slipping through after cache refresh.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 954-971: The condition on the if statement checking `oob_type and
(is_oob_ip or not has_primary_ip)` prevents OOB candidate detection when
OS/hardware information is missing but the incoming IP matches device.oob_ip.
Modify the condition to allow the OOB candidate flow to execute when is_oob_ip
is true regardless of whether oob_type could be determined, so that the logic
becomes: execute the block if is_oob_ip is true OR if oob_type is set and device
has no primary IP, effectively removing the strict requirement for oob_type when
the IP itself matches the out-of-band IP.
In `@netbox_librenms_plugin/models.py`:
- Line 940: Update the docstring at line 940 in models.py to accurately reflect
the implementation behavior. The current statement that "LAG aggregate is always
in the 'low' position of a port_stack pair" is inaccurate and misleading.
Replace it with a statement clarifying that while the LAG aggregate is typically
found in the 'low' position of a port_stack pair, the actual aggregate side is
determined by the _is_lag_aggregate() method, which means the aggregate can
appear on either the 'high' or 'low' side of the pair. This correction should
align the docstring with the actual logic implemented in the
resolve_port_relationships function.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 540-548: The rollback baseline seeding in the
select._lastVerifiedMember assignment is using || null, which converts
legitimate empty string values to null, preventing proper rollback when a verify
failure occurs from an initially unassigned member. Modify the assignment to
preserve empty string "" as a valid baseline value instead of converting it to
null. Rather than using || null as a fallback, explicitly check if the value is
undefined or use a pattern that distinguishes between "no value provided" and
"empty string selected". Apply the same fix to the additional occurrences
mentioned in the comment (lines 1169-1170).
In `@netbox_librenms_plugin/tests/test_coverage_device_fields.py`:
- Line 333: The IntegrityError exception is currently mocked on the full_clean
method instead of the save method in these save-integrity-error tests. Since
full_clean runs before save, the exception never reaches the actual save method
being tested, which means regressions in save-failure handling would not be
caught. Move the side_effect parameter with IntegrityError from the
dev.full_clean mock to the dev.save mock in both locations (line 333 and line
496) to ensure the test actually validates the save method's integrity error
handling.
In `@netbox_librenms_plugin/tests/test_data_shapes_anonymize.py`:
- Around line 233-234: The assertion is tautological because it compares a
member's serial against itself rather than verifying the master device's serial
correlation. Replace the comparison to check if the master device's serial
(accessed via result["serial"]) matches one of the member serials in the
result["members"] list. This will properly verify that the device-serial to
VC-member serial correlation is preserved after anonymization instead of always
passing due to the comparison against the same members list.
In `@netbox_librenms_plugin/tests/test_import_validation_helpers.py`:
- Line 560: The docstring starting with "Merge mode blocks import, so a stale
is_ready=True (e.g." is incomplete and cuts off mid-sentence at the opening
parenthesis. Complete this docstring by finishing the example or explanation
that was started with the (e.g. notation, providing a full sentence that
clarifies the scenario being described rather than leaving it truncated.
- Line 631: The docstring starting with "A later recalculation (e.g." is
incomplete and truncated. Complete this docstring by finishing the sentence to
fully explain the scenario being tested. The docstring should describe what the
test case is validating, including the example it mentions after "e.g." to
provide clear context for other developers reading the test.
In `@netbox_librenms_plugin/tests/test_librenms_api.py`:
- Around line 124-126: The docstring for the test method
test_get_available_servers_skips_malformed_entry is incomplete and truncated at
"A non-mapping server entry (e.g." Complete the docstring by finishing the
example with a concrete type or value that represents a malformed entry, or
alternatively remove the trailing "(e.g." fragment and revise the sentence to be
grammatically complete and descriptive of what the test validates.
🪄 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: 93b56286-e7db-40b4-88b3-168885868dbd
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (134)
.github/ISSUE_TEMPLATE/data-shape.yml.gitignoredocs/SUMMARY.mddocs/development/contributing-data-shapes.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/custom_field.mddocs/usage_tips/oob_management.mdmkdocs.ymlnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/data_shapes/__init__.pynetbox_librenms_plugin/data_shapes/anonymize.pynetbox_librenms_plugin/data_shapes/capture.pynetbox_librenms_plugin/data_shapes/recordings/cisco-lag-and-subinterface.jsonnetbox_librenms_plugin/data_shapes/recordings/cisco-stackwise-3member.jsonnetbox_librenms_plugin/data_shapes/recordings/juniper-vc-2member.jsonnetbox_librenms_plugin/data_shapes/recordings/manifest.jsonnetbox_librenms_plugin/data_shapes/recordings_store.pynetbox_librenms_plugin/data_shapes/signature.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/management/__init__.pynetbox_librenms_plugin/management/commands/__init__.pynetbox_librenms_plugin/management/commands/librenms_recordings.pynetbox_librenms_plugin/migrations/0011_portstacklagpattern.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/serial_utils.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/mappings.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_oob_interface_select.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/capture_data_shape.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern_list.htmlnetbox_librenms_plugin/tests/acs6048_sensors_fixture.jsonnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/recordings/__init__.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_cable_sync_content_template.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_data_shapes_anonymize.pynetbox_librenms_plugin/tests/test_data_shapes_capture.pynetbox_librenms_plugin/tests/test_data_shapes_command.pynetbox_librenms_plugin/tests/test_data_shapes_signature.pynetbox_librenms_plugin/tests/test_data_shapes_view.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_ipaddress_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_module_sync_content_template.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_port_stack_lag_pattern.pynetbox_librenms_plugin/tests/test_recordings.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_serial_cables_view.pynetbox_librenms_plugin/tests/test_serial_utils.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_template_comments.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_vlan_sync_content_template.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/data_shapes.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.pypyproject.toml
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/data_shapes/anonymize.py`:
- Around line 131-147: The regex pattern in _PORT_TOKEN_RE on line 147 is too
strict because it requires known prefixes from _IF_PREFIXES to be immediately
followed by an optional dash and then a digit. This causes interface names like
jsrv or jsrv.1 to fail matching, which results in them receiving independent
iface-<hash> values on line 195 instead of being recognized as related
parent-child interfaces. Modify the regex pattern in _PORT_TOKEN_RE to allow
known prefixes to match even when not immediately followed by a digit, enabling
proper parent interface identification and sub-interface correlation for the
resolver logic that follows.
In `@netbox_librenms_plugin/data_shapes/recordings/junos-subinterfaces.json`:
- Line 35: The bgpLocalAs field in the junos-subinterfaces.json fixture is set
to 6830, which is a public ASN and violates the anonymization contract for this
test data. Replace the value 6830 with a private ASN from the range 64512-65534
to properly anonymize the fixture and prevent leaking infrastructure-identifying
information.
In `@netbox_librenms_plugin/tests/test_data_shapes_anonymize.py`:
- Around line 359-381: The test_anonymization_preserves_port_relationships test
currently only validates port relationship preservation for the Cisco LAG and
sub-interface recording. Extend this test to also cover Junos by either
parameterizing the existing test to run the same _resolve logic against both a
Cisco recording (like "cisco-lag-and-subinterface") and a Junos recording, or by
adding a separate test function that performs identical relationship resolution
and assertion checks for a Junos-specific recording. This ensures that
anonymization correctly preserves port relationships for both vendors and will
catch any future anonymization regressions specific to Junos sub-interface
names.
🪄 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: cee2c91c-64f1-450b-90da-53da1c0f6317
📒 Files selected for processing (5)
netbox_librenms_plugin/data_shapes/anonymize.pynetbox_librenms_plugin/data_shapes/recordings/iosxe-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/junos-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/manifest.jsonnetbox_librenms_plugin/tests/test_data_shapes_anonymize.py
📜 Review details
🧰 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_data_shapes_anonymize.pynetbox_librenms_plugin/data_shapes/anonymize.py
🧠 Learnings (12)
📚 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_data_shapes_anonymize.pynetbox_librenms_plugin/data_shapes/anonymize.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_data_shapes_anonymize.pynetbox_librenms_plugin/data_shapes/anonymize.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_data_shapes_anonymize.pynetbox_librenms_plugin/data_shapes/anonymize.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_data_shapes_anonymize.pynetbox_librenms_plugin/data_shapes/anonymize.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_data_shapes_anonymize.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_data_shapes_anonymize.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_data_shapes_anonymize.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_data_shapes_anonymize.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_data_shapes_anonymize.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_data_shapes_anonymize.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_data_shapes_anonymize.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_data_shapes_anonymize.py
🪛 OpenGrep (1.22.0)
netbox_librenms_plugin/data_shapes/recordings/iosxe-subinterfaces.json
[ERROR] 52-52: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
marcinpsk
force-pushed
the
feat/data-shapes
branch
from
June 21, 2026 18:16
88981ef to
421d7ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/data_shapes/anonymize.py`:
- Around line 410-462: The find_pii() function currently only scans the
responses key within the recording dictionary, which means PII in top-level
fields like name, description, or meta are not checked. Modify the final scan
call at the end of the function (the line that calls scan with
recording.get("responses", {})) to scan the entire recording dictionary instead
of just the responses subset. This will ensure the PII detection safety net
covers all fields in the recording, not just nested response data.
In `@netbox_librenms_plugin/data_shapes/signature.py`:
- Line 137: The vlans computation in the line checking "ifVlan" in p or "vlans"
in p is only verifying that these keys exist in the port dictionaries, but it
should instead verify that these keys have actual non-empty values. Change the
logic to check the actual values of these keys (using p.get() or direct access
with value truthiness checks) rather than just checking for key presence with
the in operator, so that empty or null VLAN values do not incorrectly set the
vlans flag to true.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1231-1232: The CSRF token lookup using
document.querySelector('[name=csrfmiddlewaretoken]').value on line 1231 can
throw an error if the element is missing or empty, and since this happens after
row controls have already been disabled, the error will leave them stuck in a
disabled state without sending any request. Add a guard check before
constructing the request headers to verify that the CSRF token element exists
and contains a value, and perform this validation before any row control state
modifications occur.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 879-923: The click event handlers bound to dismiss buttons with
data-bs-dismiss="modal" are currently executed unconditionally as a fallback
when Bootstrap's modal JS isn't active, but the code doesn't actually check for
Bootstrap presence before binding them. To make this a true fallback, wrap the
modal.querySelectorAll click handler binding (the block that iterates over
dismiss buttons and adds click listeners) in a condition that only executes when
Bootstrap is not available or is not active. This prevents resetPromoteForm from
being called twice when Bootstrap is present—once from hidden.bs.modal and once
from the dismiss button click.
🪄 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: 021c11c6-c256-4938-acda-75c76796687b
📒 Files selected for processing (101)
.github/ISSUE_TEMPLATE/data-shape.ymldocs/SUMMARY.mddocs/development/contributing-data-shapes.mdnetbox_librenms_plugin/data_shapes/__init__.pynetbox_librenms_plugin/data_shapes/anonymize.pynetbox_librenms_plugin/data_shapes/capture.pynetbox_librenms_plugin/data_shapes/compress.pynetbox_librenms_plugin/data_shapes/recordings/arcos-lag-transceivers.jsonnetbox_librenms_plugin/data_shapes/recordings/avocent-serial-ports.jsonnetbox_librenms_plugin/data_shapes/recordings/cisco-lag-and-subinterface.jsonnetbox_librenms_plugin/data_shapes/recordings/cisco-stackwise-3member.jsonnetbox_librenms_plugin/data_shapes/recordings/iosxe-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/juniper-vc-2member.jsonnetbox_librenms_plugin/data_shapes/recordings/junos-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/linux-host-oob.jsonnetbox_librenms_plugin/data_shapes/recordings/linux-host.jsonnetbox_librenms_plugin/data_shapes/recordings/manifest.jsonnetbox_librenms_plugin/data_shapes/recordings/nokia-timos-transceivers.jsonnetbox_librenms_plugin/data_shapes/recordings_store.pynetbox_librenms_plugin/data_shapes/signature.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/management/__init__.pynetbox_librenms_plugin/management/commands/__init__.pynetbox_librenms_plugin/management/commands/librenms_recordings.pynetbox_librenms_plugin/migrations/0011_portstacklagpattern.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/serial_utils.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/mappings.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/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/capture_data_shape.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern_list.htmlnetbox_librenms_plugin/tests/acs6048_sensors_fixture.jsonnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/recordings/__init__.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_data_shapes_anonymize.pynetbox_librenms_plugin/tests/test_data_shapes_capture.pynetbox_librenms_plugin/tests/test_data_shapes_command.pynetbox_librenms_plugin/tests/test_data_shapes_compress.pynetbox_librenms_plugin/tests/test_data_shapes_signature.pynetbox_librenms_plugin/tests/test_data_shapes_view.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_port_stack_lag_pattern.pynetbox_librenms_plugin/tests/test_recordings.pynetbox_librenms_plugin/tests/test_serial_cables_view.pynetbox_librenms_plugin/tests/test_serial_utils.pynetbox_librenms_plugin/tests/test_template_comments.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_vlan_sync_content_template.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/data_shapes.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/migrate.pypyproject.toml
marcinpsk
force-pushed
the
feat/data-shapes
branch
3 times, most recently
from
June 21, 2026 20:30
bfa1708 to
b2afc4a
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
933-949:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when primary IP resolves to multiple NetBox devices.
Line 934 and Line 948 still choose a single
.first()candidate frommatching_ips. If duplicatenet_hostrows point to different devices (e.g., assigned interface on one device andoob_ipon another), this can bind/link the wrong device.Suggested fix
- matching_ips = IPAddress.objects.filter(address__net_host=primary_ip) - existing_ip = matching_ips.first() - if existing_ip: - device = ( - existing_ip.assigned_object.device - if existing_ip.assigned_object and hasattr(existing_ip.assigned_object, "device") - else None - ) - if device is None: - device = Device.objects.filter(oob_ip__in=matching_ips).first() + matching_ips = IPAddress.objects.filter(address__net_host=primary_ip) + if matching_ips.exists(): + assigned_device_ids = { + getattr(ip.assigned_object, "device_id", None) + for ip in matching_ips + if getattr(ip, "assigned_object", None) is not None + } + assigned_device_ids.discard(None) + oob_device_ids = set(Device.objects.filter(oob_ip__in=matching_ips).values_list("pk", flat=True)) + candidate_ids = assigned_device_ids | oob_device_ids + + if len(candidate_ids) > 1: + result["existing_match_type"] = "ambiguous_primary_ip" + result["can_import"] = False + result["is_ready"] = False + result["issues"].append( + f"IP address {primary_ip} maps to multiple NetBox devices; resolve duplicates before linking." + ) + return result + + device = Device.objects.filter(pk=next(iter(candidate_ids))).first() if candidate_ids else NoneAlso applies to: 965-967
🤖 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 933 - 949, The code uses `.first()` on matching_ips to arbitrarily select a single candidate when multiple IP addresses share the same net_host, which can incorrectly bind the wrong device if those IPs are assigned to different devices. To fail closed and avoid incorrect device linking, check if matching_ips has multiple results (count greater than 1) before using .first() on line 934, and if multiple matches exist, skip the device assignment for this primary_ip by logging a warning and continuing to the next iteration. Apply the same defensive check to the Device.objects.filter(oob_ip__in=matching_ips) query on line 948 to prevent selecting an incorrect OOB device when ambiguity exists.
♻️ Duplicate comments (1)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (1)
1150-1232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the CSRF lookup before aborting or locking the row.
Line 1231 can still throw after Lines 1186-1191 disable the row’s relationship buttons, so a missing/empty CSRF input strands the controls without sending a request. Resolve and validate the token before aborting the prior verification or locking controls, then reuse the local token in the header.
As per coding guidelines, all
fetch()POST requests must include a CSRF token extracted fromdocument.querySelector('[name=csrfmiddlewaretoken]').value.🐛 Proposed fix
function handleInterfaceChange(select, value) { + const csrfInput = document.querySelector('[name=csrfmiddlewaretoken]'); + const csrf = csrfInput ? csrfInput.value : ''; + if (!csrf) { + console.error('CSRF token not found. Please refresh the page and try again.'); + return; + } + // Abort any still-in-flight verification for this select: on rapid VC-member changes an // older /verify-interface/ response can otherwise arrive after a newer one and repaint the // row with stale cells/relationship controls. Mirrors handleModuleChange's AbortController. @@ headers: { 'Content-Type': 'application/json', - 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value + 'X-CSRFToken': csrf },🤖 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/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 1150 - 1232, The CSRF token lookup on line 1231 occurs after the code has already disabled the relationship buttons and locked the row controls, so if the token lookup throws an error, the controls remain stranded. Extract and validate the CSRF token from document.querySelector('[name=csrfmiddlewaretoken]').value at the very beginning of the change handler, before calling abort() on the _interfaceVerifyController or before the row querySelectorAll calls that disable the lag-sync-btn and parent-sync-btn buttons. Store the token in a local variable and reuse it in the fetch headers to ensure the token is validated before any side effects occur.Source: Coding guidelines
🤖 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/data_shapes/anonymize.py`:
- Line 147: The _IPV6_RE regex pattern only matches fully expanded IPv6
addresses and does not account for compressed IPv6 notation using the double
colon (::) shorthand, allowing compressed addresses like 2001:4860::1 to bypass
the find_pii() function. Update the _IPV6_RE regex pattern to support both fully
expanded and compressed IPv6 address formats, including the :: notation for zero
groups, ensuring all IPv6 addresses are properly detected and anonymized by the
find_pii() function.
In `@netbox_librenms_plugin/tables/interfaces.py`:
- Line 61: The `self.server_key` assignment in the constructor does not provide
a default fallback value. When `server_key` parameter is None or not provided,
assign a sensible default value (such as "default") to `self.server_key` instead
of allowing it to remain None. This ensures that when `render_librenms_id()`
method passes `self.server_key` to `get_librenms_device_id`, the helper
function's default fallback mechanism is properly utilized rather than receiving
None which bypasses the fallback logic.
In `@netbox_librenms_plugin/tests/acs6048_sensors_fixture.json`:
- Around line 7-440: The fixture file contains environment-specific and
infrastructure-identifying hostnames in the sensor_descr fields (such as
prod-lab, NL-LAB, and similar patterns) that should be pseudonymized to avoid
committing customer-identifying data. Replace all hostnames in the sensor_descr
values throughout the JSON file with generic pseudonymized names (for example,
generic labels like host-01, host-02, etc.) while preserving the Status suffix
and the overall JSON structure and formatting.
In `@netbox_librenms_plugin/tests/test_coverage_devices.py`:
- Around line 239-264: The SingleInterfaceVerifyView.post() method currently
calls get_object_or_404() before require_object_permissions_json(), allowing
device ID probing before permission validation. Reorder the code to call
require_object_permissions_json() first to check permissions, and only proceed
to get_object_or_404() after the permission gate passes, ensuring unauthorized
users cannot probe arbitrary device IDs.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 933-949: The code uses `.first()` on matching_ips to arbitrarily
select a single candidate when multiple IP addresses share the same net_host,
which can incorrectly bind the wrong device if those IPs are assigned to
different devices. To fail closed and avoid incorrect device linking, check if
matching_ips has multiple results (count greater than 1) before using .first()
on line 934, and if multiple matches exist, skip the device assignment for this
primary_ip by logging a warning and continuing to the next iteration. Apply the
same defensive check to the Device.objects.filter(oob_ip__in=matching_ips) query
on line 948 to prevent selecting an incorrect OOB device when ambiguity exists.
---
Duplicate comments:
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1150-1232: The CSRF token lookup on line 1231 occurs after the
code has already disabled the relationship buttons and locked the row controls,
so if the token lookup throws an error, the controls remain stranded. Extract
and validate the CSRF token from
document.querySelector('[name=csrfmiddlewaretoken]').value at the very beginning
of the change handler, before calling abort() on the _interfaceVerifyController
or before the row querySelectorAll calls that disable the lag-sync-btn and
parent-sync-btn buttons. Store the token in a local variable and reuse it in the
fetch headers to ensure the token is validated before any side effects occur.
🪄 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: fe89dcf0-8d71-4d6b-9057-9bc1790e40b4
📒 Files selected for processing (99)
.github/ISSUE_TEMPLATE/data-shape.ymldocs/SUMMARY.mddocs/development/contributing-data-shapes.mdnetbox_librenms_plugin/data_shapes/__init__.pynetbox_librenms_plugin/data_shapes/anonymize.pynetbox_librenms_plugin/data_shapes/capture.pynetbox_librenms_plugin/data_shapes/compress.pynetbox_librenms_plugin/data_shapes/recordings/arcos-lag-transceivers.jsonnetbox_librenms_plugin/data_shapes/recordings/avocent-serial-ports.jsonnetbox_librenms_plugin/data_shapes/recordings/cisco-lag-and-subinterface.jsonnetbox_librenms_plugin/data_shapes/recordings/cisco-stackwise-3member.jsonnetbox_librenms_plugin/data_shapes/recordings/iosxe-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/juniper-vc-2member.jsonnetbox_librenms_plugin/data_shapes/recordings/junos-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/linux-host-oob.jsonnetbox_librenms_plugin/data_shapes/recordings/linux-host.jsonnetbox_librenms_plugin/data_shapes/recordings/manifest.jsonnetbox_librenms_plugin/data_shapes/recordings/nokia-timos-transceivers.jsonnetbox_librenms_plugin/data_shapes/recordings_store.pynetbox_librenms_plugin/data_shapes/signature.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/management/__init__.pynetbox_librenms_plugin/management/commands/__init__.pynetbox_librenms_plugin/management/commands/librenms_recordings.pynetbox_librenms_plugin/migrations/0011_portstacklagpattern.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/serial_utils.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/mappings.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/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/capture_data_shape.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern_list.htmlnetbox_librenms_plugin/tests/acs6048_sensors_fixture.jsonnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/recordings/__init__.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_data_shapes_anonymize.pynetbox_librenms_plugin/tests/test_data_shapes_capture.pynetbox_librenms_plugin/tests/test_data_shapes_command.pynetbox_librenms_plugin/tests/test_data_shapes_compress.pynetbox_librenms_plugin/tests/test_data_shapes_signature.pynetbox_librenms_plugin/tests/test_data_shapes_view.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_port_stack_lag_pattern.pynetbox_librenms_plugin/tests/test_recordings.pynetbox_librenms_plugin/tests/test_serial_cables_view.pynetbox_librenms_plugin/tests/test_serial_utils.pynetbox_librenms_plugin/tests/test_template_comments.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_vlan_sync_content_template.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/data_shapes.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/migrate.pypyproject.toml
marcinpsk
force-pushed
the
feat/data-shapes
branch
from
June 22, 2026 04:23
b2afc4a to
913d802
Compare
marcinpsk
commented
Jun 22, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/data_shapes/recordings_store.py`:
- Around line 45-48: The try-except block in the load_manifest() function only
catches ValueError when parsing JSON, but the MANIFEST_PATH.read_text() call can
also raise OSError due to file permission or I/O issues. Modify the except
clause to catch both ValueError and OSError exceptions, ensuring that both JSON
decode failures and file read failures return an empty list as intended by the
best-effort design of load_manifest().
In `@netbox_librenms_plugin/data_shapes/signature.py`:
- Line 138: The transceivers field assignment on line 138 currently validates
the presence of any body response from endpoints matching "/transceivers", which
incorrectly includes error responses and 404 bodies. Instead of checking if the
body is not None, modify the logic to verify that the transceivers list in the
response payload is actually non-empty, ensuring only legitimate transceiver
data counts toward transceiver coverage detection rather than error responses.
In `@netbox_librenms_plugin/management/commands/librenms_recordings.py`:
- Around line 64-65: The error message being raised in the CommandError includes
the actual PII values by referencing p['value'] in the format string when
building the lines variable. This causes sensitive data to be logged in CI
output. Remove the p['value'] reference from the format string that creates the
lines variable, keeping only the p['kind'] and p['path'] information, which is
sufficient to identify where PII was found without exposing the actual sensitive
values.
- Around line 29-50: The add_arguments method registers --validate,
--rebuild-manifest, and --list as independent arguments without constraints,
allowing them to be combined, while the handle method silently executes only the
first matching option via if/elif logic, creating ambiguous CLI behavior.
Instead of adding these three arguments directly to the parser in add_arguments,
create a mutually exclusive group using parser.add_mutually_exclusive_group()
and add all three arguments (--validate, --rebuild-manifest, and --list) to that
group. This will prevent users from combining these options and make the CLI
behavior explicit and unambiguous.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 2309-2315: The fallback logic in the objectId assignment is
incorrectly treating an empty vcMemberSelect value as falsy and falling back to
btn.dataset.objectId, which causes stale member sync to the previous value
instead of preserving the intentional empty selection. Fix the ternary operator
that assigns objectId to check whether vcMemberSelect exists (not whether it has
a truthy value), so that an empty select value is properly preserved. Use
vcMemberSelect.value whenever vcMemberSelect itself exists as an element,
regardless of whether its value is empty, and only fall back to
btn.dataset.objectId when vcMemberSelect does not exist.
🪄 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: 29322b0a-4a20-4479-a4eb-fd97858bf560
📒 Files selected for processing (97)
.github/ISSUE_TEMPLATE/data-shape.ymldocs/SUMMARY.mddocs/development/contributing-data-shapes.mdnetbox_librenms_plugin/data_shapes/__init__.pynetbox_librenms_plugin/data_shapes/anonymize.pynetbox_librenms_plugin/data_shapes/capture.pynetbox_librenms_plugin/data_shapes/compress.pynetbox_librenms_plugin/data_shapes/recordings/arcos-lag-transceivers.jsonnetbox_librenms_plugin/data_shapes/recordings/avocent-serial-ports.jsonnetbox_librenms_plugin/data_shapes/recordings/cisco-lag-and-subinterface.jsonnetbox_librenms_plugin/data_shapes/recordings/cisco-stackwise-3member.jsonnetbox_librenms_plugin/data_shapes/recordings/iosxe-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/juniper-vc-2member.jsonnetbox_librenms_plugin/data_shapes/recordings/junos-subinterfaces.jsonnetbox_librenms_plugin/data_shapes/recordings/linux-host-oob.jsonnetbox_librenms_plugin/data_shapes/recordings/linux-host.jsonnetbox_librenms_plugin/data_shapes/recordings/manifest.jsonnetbox_librenms_plugin/data_shapes/recordings/nokia-timos-transceivers.jsonnetbox_librenms_plugin/data_shapes/recordings_store.pynetbox_librenms_plugin/data_shapes/signature.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/management/__init__.pynetbox_librenms_plugin/management/commands/__init__.pynetbox_librenms_plugin/management/commands/librenms_recordings.pynetbox_librenms_plugin/migrations/0011_portstacklagpattern.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/serial_utils.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/mappings.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/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/capture_data_shape.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern_list.htmlnetbox_librenms_plugin/tests/acs6048_sensors_fixture.jsonnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/recordings/__init__.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_data_shapes_anonymize.pynetbox_librenms_plugin/tests/test_data_shapes_capture.pynetbox_librenms_plugin/tests/test_data_shapes_command.pynetbox_librenms_plugin/tests/test_data_shapes_compress.pynetbox_librenms_plugin/tests/test_data_shapes_signature.pynetbox_librenms_plugin/tests/test_data_shapes_view.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_port_stack_lag_pattern.pynetbox_librenms_plugin/tests/test_recordings.pynetbox_librenms_plugin/tests/test_serial_cables_view.pynetbox_librenms_plugin/tests/test_serial_utils.pynetbox_librenms_plugin/tests/test_template_comments.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_vlan_sync_content_template.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/data_shapes.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/migrate.pypyproject.toml
marcinpsk
force-pushed
the
feat/data-shapes
branch
4 times, most recently
from
June 22, 2026 11:33
bc49725 to
00d5ec3
Compare
sysDescr is scrubbed free text (FREETEXT_KEYS), not a version/model identifier, so an IP/FQDN surviving in it (a field-rule miss) is a real leak the safety net must flag — yet it was in _IP_EXEMPT_KEYS, suppressing exactly that check. This contradicted the module's own entPhysicalDescr reasoning (free-text-carrying-address fields must stay scannable). Remove sysDescr from the exempt set; committed recordings stay clean. Red->green test.
The recording schema doesn't constrain either key, so a community-submitted
meta: "garbage" or lag_patterns: [] passed the or-{} guard (truthy) and
crashed compute_shape_signature with AttributeError during --validate instead
of degrading like the null case. isinstance-normalize both once; oob_present
reuses the normalized meta.
…logical Comparing member serials against members[0]'s own serial always passes; the assertion now compares against the anonymized device endpoint's serial, so it actually pins that anonymization preserves the device-serial ↔ member-serial correlation detect_virtual_chassis_from_inventory keys the master on.
…heck bool is a subclass of int, so `schema_version == 1` and isinstance(device_id, int) alone would let True/False satisfy the contract check even though recording_schema_errors() rejects them. Assert not-a-bool for both, matching the production validator.
…lpers
recording.get("meta", {}) only falls back on an ABSENT meta; an explicit
"meta": null (a shape recording_schema_errors() accepts) returns None and the
chained .get("os")/.get("oob_id") would AttributeError. Normalize with
(meta or {}) so a future community recording with meta:null replays cleanly.
CaptureDataShapeView resolved the target Device through the plain manager (get_object_or_404), but require_object_permissions only checks model-level has_perm with no instance — a permission a site-constrained view_device grant passes. So a user scoped to one site could capture any device's LibreNMS data shape by raw pk, including the raw IP/MAC values the modal surfaces via find_pii. Resolve through restrict_object_or_404 instead (the mixin the view already inherits), so an out-of-scope id 404s exactly like a nonexistent one — matching ip_addresses_view. Add a red-green test driving a real constrained ObjectPermission (the model-level gate passes, the object is out of scope) rather than a mocked has_perm, which would hide the missing object-scoping entirely.
Five corrections to compute_shape_signature / classify_novelty, all verified
red-green; the manifest is rebuilt for the axis changes (only a per-entry
"serial" flag is added — no lag/name_prefix change, so no shipped signature
shifts):
- Add a serial-port axis. A capture with recognized serial sensors and a plain
host of the same OS/VC/LAG/sub shape had identical signatures, so the first
Avocent serial shape would report as already covered by a non-serial sibling
(mirrors the oob axis).
- Consume lag.ieee8023ad in classify_novelty. The signature distinguishes an
ifType LAG from a pattern-only LAG, but _structural_axes dropped it, so the
two collapsed into one novelty bucket.
- Ignore failed response frames. _body unwrapped a [status, body] pair without
checking the status, so a captured [500, {...}] counted as present data that a
replay can never reproduce; it now yields None for a non-2xx frame.
- Strip a sub-unit suffix from name_prefix before the aggregate number, so a
first LAG port of ae1.0 yields the convention "ae" (like ae2.0), not "ae1.".
- Refuse ReDoS-prone lag_patterns. An untrusted community pattern is compiled and
matched by --validate in CI; the length cap cannot bound catastrophic
backtracking (^(a+)+$ is exponential), so skip a pattern with a nested
unbounded quantifier / over-long / non-string, and cap the pattern count. A
heuristic, not a guarantee — human review and the CI timeout remain the gates.
…atterns Two capture-fidelity fixes, both verified red-green: - Fail the capture on an HTTP 5xx from the /all inventory fallback. Once the filtered query returns empty, /all is the only inventory source; a server error there means the topology is unknown, but capture treated any real HTTP status (incl. 5xx) as a definitive "no inventory" and shipped a VC device as a plain one. A 404 / 2xx-empty stays a genuine "no inventory" for a plain device. - Snapshot serial_type_patterns only when the device actually has serial sensors. The map is read on replay solely to reproduce serial rows, so embedding the whole (possibly operator-customized) recognition table into every no-sensor recording was dead weight and needless exposure — now gated on sensor presence, matching the bundled recordings (only the serial capture carries it).
The residual-PII scan required a dotted domain, so an internal-domain email like "netops@corp" in a preserved free-text field (entPhysicalName/Descr) slipped through. Make the domain dot(s) optional; the local part before @ is still required, so a bare @handle or a spaced "x @ y" does not match. Red-green test on a preserved field.
The dotless-email relaxation introduced an exponential-backtracking regex (CodeQL py/redos, high): in `(?:\.[\w.-]+)*` the label class `[\w.-]` overlaps the `\.` separator, so a run of ".-.-.-" repartitions combinatorially. Make each label `[\w-]+` (no dot), so `.` is an unambiguous separator, and bound the local part to the RFC 5321 max of 64 so a long no-`@` run can't backtrack quadratically across word boundaries. The regex is now linear (a 10k-char adversarial input resolves in <1ms) and still matches the same emails (dotless `user@host` and normal dotted forms). Verified with a red-green ReDoS-timing test; solution formulation validated by an independent adversarial review.
…alt each capture
Four fidelity/privacy fixes on the capture pipeline:
- The capture view anonymized with the empty default salt, so every pseudonym was
a plain sha256 of the underlying value. The contributor publishes that file, and
hostnames/serials are low-entropy enough to confirm by hashing candidates. Pass a
fresh per-capture salt: pseudonyms only need to be consistent within one
recording, which is what keeps cross-references matching.
- The compressor's port fingerprint carried ifType, sub-interface naming and VLAN
presence, but not the name-pattern LAG axis the signature also reads. A Cisco
"Po1" (propVirtual, recognized by name) therefore shared a fingerprint with any
other propVirtual port, and compression could drop the device's only LAG port,
breaking the module's own signature-preservation invariant.
- The anonymizer hashed any interface name its built-in prefix list did not know,
including names the recording's OWN lag_patterns match. The surviving pattern
then matched nothing, so replay no longer saw the aggregate. Such a name is kept
verbatim, bounded by a port-name shape guard so a sloppy operator pattern cannot
turn the rule into a free-text leak.
- Serial default labels were recognized by a hard-coded prefix list, while what
counts as "default" is defined by the recording's serial_type_patterns (the
seeded Cisco map is "Line {N}"). Such labels were pseudonymized, flipping
is_configured False -> True on replay. The matchers are now built from those
patterns, with only {N} becoming a quantifier.
The shared port primitives (names, VLAN predicate, LAG pattern compilation and
matching) move to data_shapes/ports.py: signature, compress and anonymize all need
them, and anonymize importing signature would have closed an import cycle. This
also removes compress's duplicate copy of the name accessor.
… capture Serial cable sync removed the use_cache parameter from get_serial_port_sensors when it stopped caching the instance-wide sensor table. The capture path still passed use_cache=False, so every capture that reached the serial-sensor route died with TypeError: got an unexpected keyword argument. The method now fetches the sensor table on every call, so the cache bypass the capture wanted is the default. The stale-cache regression test still poisons the per-server key and asserts the capture ignores it, which is the behaviour worth guarding; only its wording referred to the removed argument.
capture_device_recording snapshots the device OS's PortStackLagPattern rows into recording["lag_patterns"], and both compute_shape_signature and the replay's resolve_port_relationships read that key. The bundled recordings predate the feature: nine carried no key at all and two carried an empty map. With the key absent the resolver falls back to a database lookup, which the data-shapes tests cannot reach, so every pattern-based LAG relationship resolved empty. Each recording now carries the map capture would have written: the OS filtered set from migration 0013, with the key pseudonymized exactly as anonymize.py does. pseudonymize_os is salt-independent and stable, so the real OS behind each os-<hash> token is recoverable and the mapping is verifiable rather than guessed. An OS with no configured pattern gets an empty map, which is what capture produces and is distinct from the absent key that triggers the database fallback. The manifest moves with it: iosxe-subinterfaces now fingerprints as lag.present=true with prefix "Po" instead of lag.present=false, which is the pattern-LAG behaviour the recording was captured for.
…ource The regression check asserted two exact source substrings, so it passed on any build and broke on a reformat without saying anything about behavior. A browser test now clicks the button with a throwing execCommand and asserts the throwaway textarea is gone; removing the finally block turns it red. The source-text test is dropped because it covered nothing this does not.
delete navigator.clipboard is a no-op because clipboard is an accessor on
Navigator.prototype; the fallback ran only because an insecure context does not
expose the API. The test now removes it outright, asserts execCommand('copy')
was called once, and closes the browser from a finally block.
The suite now has one place that opens and closes a browser, so the local try/finally is redundant.
Continue the convention restore on the production files this branch changes. Refs #117
Continue the one-line test convention on the test files this branch changes. Refs #117
test_mapping_without_both_port_ids_fails_closed now serves its payload over real HTTP like the rest of its class; removing the both-port-ids guard turns both parametrized cases red. The two virtual-chassis recording tests patched the member-name pattern loader. They now write a real LibreNMSSettings row, so the real loader runs. The mid-capture transport failure moved from patching capture_device_recording to refusing the /ports request at the HTTP boundary, so the real capture code handles the failure. test_load_manifest_returns_empty_on_unreadable_file now points at a real directory where the manifest file is expected, so the real read raises. The recording-API delegation test keeps its wrapper: it is the only observable seam while the config is patched, and it already calls the real constructor. Refs #117
Both patches in this module replaced get_plugin_config with a two-argument lambda. librenms_api calls it with a default as well (cache_timeout at line 49), so any path reaching that call under these patches raises TypeError, and every non-servers key returned None instead of its real setting. Take *args/**kwargs and delegate everything except "servers" to the real function, matching make_recording_api in conftest.
Three assertions were shaped so they could not fail for the reason their names
give.
Both `find_pii` secret tests compared the finding list against a literal whose
`path` was read back out of the result: `[{"path": secret_findings[0]["path"],
…}]`. The reported path was therefore never checked, and an empty result raised
IndexError instead of failing readably. Each now asserts the count first (with
the findings as the message), then kind, value, and the path separately.
Reporting a wrong path now fails on the path assertion.
`test_capture_serial_sensors_bypasses_stale_cache` seeded
`librenms_serial_sensors_{server_key}` and asserted the capture ignored it. No
production code reads that key: `get_serial_port_sensors()` fetches the
instance-wide sensor table on every call, which capture.py:205 already states in
a comment. The poisoned entry was inert, so the test passed whatever the capture
did with the cache. It now counts requests to `/api/v0/resources/sensors`
through a wrapped route: it reads once, clears the counter, then captures and
asserts the capture hit the server itself. Renamed to
`test_capture_serial_sensors_reads_the_server_every_time`, which is what it can
actually prove. Stubbing out the capture's `get_serial_port_sensors()` call
turns it red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #104 — review only the delta over
feat/serial-ports.Data-shape capture and replay testing. Capture a device's real LibreNMS responses (UI modal +
librenms_recordingsmanagement command), run them through a deterministic field-aware anonymizer + residual-PII scanner, classify the shape's novelty against a signature manifest, and replay bundled recordings as outcome tests (a recording with anexpectedblock becomes a test with no new code). Ships 10 anonymized recordings; only the manifest is packaged in the wheel.Motivation / Problem
Feature (#95). Test real LibreNMS data shapes without committing customer data.
Scope of Change
How Was This Tested?
Risk Assessment
Test/tooling and an opt-in capture button; no change to sync/import behaviour.
Backwards Compatibility
Summary by CodeRabbit
New Features
Documentation
Bug Fixes