Feat: per vendor fetch#74
Merged
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 8 minutes and 51 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
WalkthroughThis PR adds a complete NetBox export-diff workflow with image verification caching, manufacturer-scoped GraphQL querying, vendor-scoped component preloading, per-vendor import orchestration, and an unmanaged-type removal flag. It includes new serialization, manifest persistence, GraphQL error handling, vendor lookup utilities, and CLI refactoring, with comprehensive test coverage. ChangesExport-diff and image verification features
Change detection, repo fast-paths, and CLI orchestration
Test coverage and documentation
Estimated code review effort: Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
111-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new export-diff CLI flags in README.
The CLI now exposes
--export-diff,--export-diff-dir, and--force-export-overwrite, but they’re missing from the “All Arguments” table and follow-up usage docs. Please add them so help text and README stay aligned.🤖 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 `@README.md` around lines 111 - 125, Add the three new CLI flags to the "All Arguments" table and to any follow-up usage sections: document `--export-diff` (off by default; export a diff of changes instead of applying), `--export-diff-dir` (path where diffs are written), and `--force-export-overwrite` (overwrite existing diff files). Update the table rows to match the existing format (Argument | Default | Description) and add brief descriptions for each flag in the usage docs so the README aligns with the CLI help output.
🤖 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 `@core/export_manifest.py`:
- Around line 18-20: load_manifest currently returns any JSON value which breaks
later .get(...) calls; update load_manifest to validate and normalize the parsed
JSON: after reading json.loads, ensure the result is a dict, then build and
return {k: (value if isinstance(value, dict) else {}) for k in _EMPTY} using
values from the parsed dict (or empty dict when missing), and preserve the
existing except handling for FileNotFoundError/json.JSONDecodeError by returning
the same normalized default mapping; reference load_manifest and _EMPTY when
locating the code to change.
In `@core/export.py`:
- Around line 365-369: The current vendor filter compares CLI slugs in
self.vendor_slugs to raw directory names (wanted vs d.name.lower()), which fails
when folder names contain spaces or different punctuation (e.g., "Extreme
Networks" vs "extreme-networks"); implement a slug-normalization step (e.g., a
slugify helper used by the method that lowercases, replaces non-alphanumeric
chars with '-', collapses multiple '-' and strips edges) and compare wanted
slugs to slugified directory names (use the existing vendor_slugs set and
replace d.name.lower() with slugify(d.name) in the iteration so yield d only
when slugified name is in wanted).
In `@core/nb_serializer.py`:
- Around line 202-205: The code sets result["front_image"] and
result["rear_image"] directly using attribute access on nb_record which is
inconsistent and can raise AttributeError; update those checks to use
getattr(nb_record, "front_image", None) and getattr(nb_record, "rear_image",
None) (e.g., treat non-None as present) so the nb_record/front_image and
nb_record/rear_image checks are robust and consistent with how GraphQL fields
are handled elsewhere like in core/graphql_client.py.
In `@core/netbox_api.py`:
- Around line 1364-1369: filter_actionable_module_types() currently only treats
module types as actionable when an attachment basename exists in NetBox, but
verify_images and the richer data from
get_module_type_image_details()/self._module_image_details/module_type_existing_images
are not consulted; update filter_actionable_module_types to use the cached
self._module_image_details (or call get_module_type_image_details() if not
cached) and mark a module type actionable when the remote image is missing or
when the remote image metadata (e.g., checksum or size from
module_type_existing_images) differs from the local bytes—concretely, augment
the basename existence check in filter_actionable_module_types to also compare
checksum/byte-size fields from self._module_image_details for each attachment
and treat mismatches as actionable so --verify-images produces the richer
verification behavior.
- Around line 796-802: When an existing image passes verification and you skip
uploading, seed the hash cache so future local edits will be detected: after the
"verified OK" branch in the block that uses
_is_image_hash_changed(saved_images[image_kind], self._image_hash_cache) (the
code that logs "{label} verified OK for {dt.model}, skipping upload." and
deletes saved_images[image_kind]), set self._image_hash_cache[image_kind] =
saved_images[image_kind] (or otherwise persist the same hash value that was used
for verification) before deleting from saved_images; apply the same change to
the analogous verification block around the _is_image_hash_changed call at the
other location mentioned.
- Around line 82-85: In _check_image_url replace the hard-coded Authorization
header with a call to _build_auth_header(token) so nbt_... tokens get the
correct Bearer scheme, and when image_url_path is an absolute URL only include
that auth header if the target host matches the configured base_url host (to
avoid sending credentials to off-host URLs); otherwise omit the Authorization
header but keep verify/timeout behavior and use the computed full_url as before.
- Around line 121-137: Replace unsafe pickle usage in _load_image_hash_cache and
_save_image_hash_cache with a JSON-based implementation: serialize the cache
dict to JSON in _save_image_hash_cache and load via json.load in
_load_image_hash_cache, validating the result is a dict of str->str (and
returning {} on any parse/validation/IO error). In addition, update the
NetBox.__init__ image-cache load path (the load around the current
NetBox.__init__ code that calls _load_image_hash_cache) to attempt JSON first
and, for backward compatibility, detect a legacy pickle file and only then
safely attempt to read it using a non-executing fallback (e.g., open and reject
pickle content) or migrate it by refusing to unpickle and logging/informing to
regenerate the cache; ensure all error cases return an empty dict and that file
writes atomically replace the JSON file.
In `@core/repo.py`:
- Line 451: DTLRepo.resolve_slug_files and
core.netbox_api._load_image_hash_cache currently call pickle.load on files
coming from cloned repos, which allows RCE; replace these unsafe loads by
switching the stored format to a non-executable serialization (e.g., JSON or
YAML) and update the code to use json.load/json.loads (or a safe YAML loader)
for tests/known-*.pickle files, or if format migration is not possible implement
a strict restricted unpickler that only permits basic builtins and explicit
types before calling pickle.load; update file names/extension handling in
DTLRepo.resolve_slug_files and _load_image_hash_cache and add validation that
the file is inside the expected repo test directory and has expected
structure/schema before trusting contents.
In `@nb-dt-import.py`:
- Around line 1052-1057: When args.vendors is given we currently build
vendor_slug_filter and set vendors_to_process from all_vendors but never handle
the case where no slugs matched; add an explicit check after computing
vendors_to_process (using the existing variables vendor_slug_filter,
vendors_to_process, and all_vendors) and if vendors_to_process is empty, log a
clear error or warning indicating the requested vendor slugs were not found and
exit early (non-zero) to avoid a silent no-op.
In `@tests/test_exporter.py`:
- Around line 21-24: The test settings fixture is setting s.TOKEN but Exporter
reads settings.NETBOX_TOKEN; update the fixture to set s.NETBOX_TOKEN (leave
s.IGNORE_SSL_ERRORS and s.REPO_PATH unchanged) so tests exercise the same config
name used by Exporter and avoid hiding regressions related to NETBOX_TOKEN.
In `@tests/test_nb_dt_import.py`:
- Around line 1334-1338: The test uses a hardcoded absolute cwd in the
subprocess.run call (the invocation that runs "uv run ... nb-dt-import.py")
which breaks CI; change it to compute a portable working directory at runtime
(e.g. locate the repository or the test file directory via pathlib/pytest
fixtures) and pass that path to subprocess.run instead of
"/home/mzieba/workspace/Device-Type-Library-Import"; update each occurrence in
tests/test_nb_dt_import.py that calls subprocess.run for "nb-dt-import.py" (the
calls around the shown block and the ones at lines ~1345-1350, 1355-1360,
1365-1370) to use the computed relative/pathlib-based cwd or omit cwd and pass
the full script path so the tests run on any machine.
---
Outside diff comments:
In `@README.md`:
- Around line 111-125: Add the three new CLI flags to the "All Arguments" table
and to any follow-up usage sections: document `--export-diff` (off by default;
export a diff of changes instead of applying), `--export-diff-dir` (path where
diffs are written), and `--force-export-overwrite` (overwrite existing diff
files). Update the table rows to match the existing format (Argument | Default |
Description) and add brief descriptions for each flag in the usage docs so the
README aligns with the CLI help output.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2d8a259-c558-468c-a34a-32ff207e7e48
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignoreCHANGELOG.mdREADME.mdcore/change_detector.pycore/export.pycore/export_manifest.pycore/graphql_client.pycore/nb_serializer.pycore/netbox_api.pycore/repo.pynb-dt-import.pytests/test_change_detector.pytests/test_export_manifest.pytests/test_exporter.pytests/test_graphql_client.pytests/test_nb_dt_import.pytests/test_nb_serializer.pytests/test_netbox_api.pytests/test_repo.py
This was referenced May 22, 2026
marcinpsk
added a commit
that referenced
this pull request
May 22, 2026
Fixes:
- export_manifest: validate load_manifest JSON is dict; reset non-dict
sections to {} instead of crashing on .get() calls (CB #3288667922)
- export: slugify vendor dir names before matching against CLI slugs so
'Extreme Networks' -> 'extreme-networks' works correctly (CB #3288667947)
- netbox_api: _check_image_url now uses _build_auth_header (Bearer for
nbt_ tokens) and suppresses auth header for off-host image URLs to
prevent credential leakage (CB #3288667959)
- netbox_api: seed image hash cache when verify-images reports OK with
no upload so future local edits are detected (CB #3288667974)
- nb_serializer: use getattr(..., None) for front_image/rear_image to
avoid AttributeError on partial records (CB #3288667954)
- nb-dt-import: exit(1) with clear message when --vendors slug matches
nothing instead of silently succeeding (CB #3288667991)
- tests/test_exporter: fixture TOKEN -> NETBOX_TOKEN (CB #3288668000)
- tests/test_nb_dt_import: replace hardcoded cwd with _PROJECT_ROOT so
CI subprocess tests work on any machine (CB #3288668013)
New tests (+13):
- TestLoadManifest: non-dict JSON (list, string), non-dict section,
missing sections filled (CB #3288667922)
- TestVendorDirSlugNormalization: single-word, multi-word/hyphenated,
exclusion, no-filter, nonexistent root (CB #3288667947)
- TestCheckImageUrl: Bearer nbt_ token, off-host auth suppression,
renamed existing Token test for clarity (CB #3288667959)
- TestVerifyImagesDeviceType: verify-OK seeds hash cache (CB #3288667974)
- TestMain: unknown --vendors exits nonzero (CB #3288667991)
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
core/repo.py (1)
477-515:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate pickle entry shape and constrain resolved paths to repo root.
resolve_slug_files()trusts pickle payload structure and joinsrelpathdirectly (Line 485). Malformed entries can crash unpacking, and..segments can escape the repository root.🔒 Suggested hardening
slugs_lower = [s.casefold() for s in slugs] + repo_root_real = os.path.realpath(repo_root) # --- device types -------------------------------------------------- device_files = {} # vendor_slug -> [abs_path] try: known_slugs = _safe_pickle_load(device_pickle) except Exception: return None - for entry_slug, relpath in known_slugs: + for entry in known_slugs: + if not (isinstance(entry, (tuple, list)) and len(entry) == 2): + continue + entry_slug, relpath = entry + if not isinstance(entry_slug, str) or not isinstance(relpath, str): + continue if not any(s in entry_slug.casefold() for s in slugs_lower): continue parts = relpath.replace("\\", "/").split("/") - if len(parts) < 3: + if len(parts) < 3 or parts[0] != "device-types": continue vendor_name = parts[1] vendor_slug = self.slug_format(vendor_name) - abs_path = os.path.join(repo_root, *parts) + abs_path = os.path.realpath(os.path.join(repo_root, *parts)) + if not abs_path.startswith(repo_root_real + os.sep): + continue device_files.setdefault(vendor_slug, []).append(abs_path) # --- module types -------------------------------------------------- module_vendors = set() if os.path.exists(module_pickle): try: known_modules = _safe_pickle_load(module_pickle) except Exception: known_modules = set() - for model_name, vendor_dir in known_modules: + for entry in known_modules: + if not (isinstance(entry, (tuple, list)) and len(entry) == 2): + continue + model_name, vendor_dir = entry + if not isinstance(model_name, str) or not isinstance(vendor_dir, str): + continue if not any(s in model_name.casefold() for s in slugs_lower): continue vendor_name = vendor_dir.replace("\\", "/").split("/")[-1] module_vendors.add(self.slug_format(vendor_name))🤖 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 `@core/repo.py` around lines 477 - 515, In resolve_slug_files(), validate pickle entry shapes and constrain any joined paths to the repo root: when iterating known_slugs, ensure each entry unpacks safely (e.g., check type and length before doing "for entry_slug, relpath in known_slugs") and skip malformed entries; normalize relpath and vendor_dir with os.path.normpath, then join with repo_root and verify using os.path.commonpath (or equivalent) that the resulting abs_path remains inside repo_root before appending to device_files; apply the same shape-checking and path-normalization/containment checks for known_modules and known_racks (when reading module_pickle and rack_pickle) and skip entries that don't meet the expected tuple shape or that resolve outside the repository.core/export.py (1)
158-345:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRefactor
Exporter.run()to reduce complexity and pass Ruff C901.Line 158 currently exceeds the enforced cyclomatic threshold (23 > 15), so CI fails. Please split orchestration into smaller helpers (e.g., vendor collection/comparison, rack comparison, write phase).
🤖 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 `@core/export.py` around lines 158 - 345, Exporter.run currently has too high cyclomatic complexity (C901); refactor by extracting logical blocks into smaller helper methods to reduce branching inside run. Split at least these sections into new private methods on the same class: _collect_vendor_slugs_and_records (build dt_by_vendor, mt_by_vendor, all_vendor_slugs), _compare_vendor_and_enqueue_items (the per-vendor stale checks, _fetch_vendor_components and calls to _determine_export_set_for_device_types/_module_types), _compare_rack_types (rack_records loop and _determine_export_set_for_rack_types), and _write_export_items (the write_task loop including _write_yaml, _download_type_images and manifest updates). Keep run as an orchestrator that calls these helpers in sequence and moves local variables (items, skipped_fresh, manifest_path, progress) into arguments/returns as needed so each helper is small and the overall cyclomatic complexity of run falls below the threshold.nb-dt-import.py (2)
897-1173: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSplit
main()before merge.CI is already failing on C901 here.
main()now owns CLI validation, setup, vendor selection, progress lifecycle, and the full per-vendor import flow, so extracting the vendor loop and the bootstrapping/finalization steps into helpers is needed to get back under the complexity threshold.🤖 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 `@nb-dt-import.py` around lines 897 - 1173, The main() function is too large and triggers C901; split it by extracting the vendor-processing loop and progress lifecycle into one or two helper functions (e.g., create a function process_vendors(args, settings, dtl_repo, netbox, handle) to encapsulate the for vendor in vendors_to_process loop and its inner logic that calls _process_device_types, _process_module_types, _process_rack_types, netbox.load_vendor, netbox.create_manufacturers, and progress/task_registry management; and another helper bootstrap_and_validate(args, settings, handle) to perform CLI validation, arg normalization, NetBox/DTLRepo instantiation, _apply_slug_fast_path and vendor discovery); ensure main() retains only high-level orchestration and calls get_progress_panel and _finalize_task_registry as before, passing necessary objects (dtl_repo, netbox, progress, task_registry) into the new helpers so behavior and side-effects remain identical.
867-877:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFall back when the slug fast-path resolves nothing.
This branch makes the pickle index authoritative: if it returns no matching vendors, the import becomes a false-success no-op even though the checked-out repo may still contain matching YAML. Treat an empty fast-path result as a cache miss and fall back to the normal scan instead of narrowing to
[].Suggested minimal fix
matched_vendor_slugs = ( set(slug_resolved["device_files"]) | slug_resolved["module_vendors"] | slug_resolved["rack_vendors"] ) + if not matched_vendor_slugs: + handle.verbose_log("Slug pickle produced no matches; falling back to full file scan.") + return vendors_to_process, None narrowed = [v for v in vendors_to_process if v["slug"] in matched_vendor_slugs] handle.verbose_log( f"Slug pickle resolved {sum(len(f) for f in slug_resolved['device_files'].values())} " f"device file(s) across {len(matched_vendor_slugs)} vendor(s)." )🤖 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 `@nb-dt-import.py` around lines 867 - 877, The current fast-path makes the pickle index authoritative and returns an empty narrowed list when matched_vendor_slugs is empty; change the logic so that if matched_vendor_slugs is empty (i.e., no picks from slug_resolved), treat it as a cache miss and fall back to the normal scan by setting narrowed to vendors_to_process instead of [], and update the verbose_log message accordingly; locate the block that computes matched_vendor_slugs and narrowed (references: matched_vendor_slugs, slug_resolved, vendors_to_process, narrowed, handle.verbose_log) and implement the conditional: if not matched_vendor_slugs: narrowed = vendors_to_process (and log a cache-miss message) else keep the current narrowing and log the resolved counts.
🤖 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 `@core/repo.py`:
- Line 3: Remove the unused import by deleting the top-level "import io"
statement in core/repo.py; this eliminates the Ruff F401 unused-import error and
keeps the module imports minimal (no other code changes needed).
In `@tests/test_change_detector.py`:
- Around line 638-639: Add a simple class docstring to the public test class
TestLogChangeReportNoModified to satisfy Ruff D101; update the class definition
containing the method test_logs_zero_modified_when_only_new_types_exist by
inserting a brief descriptive string (one-line docstring) immediately below the
class declaration that explains the purpose of the test class.
In `@tests/test_exporter.py`:
- Around line 173-174: Add concise class-level docstrings to the test classes
missing them (e.g., TestYamlEqual) to satisfy Ruff D101; for each test class
mentioned in the review, open the class declaration (for example class
TestYamlEqual:) and add a one-line docstring immediately under the class header
("""Tests for ...""" or similar), and repeat the same minimal docstring pattern
for the other flagged test classes referenced in the comment so CI linting
passes.
- Line 641: Long dictionary literal lines (e.g., the mapping for the key
("Nokia", "R-SAME") in tests/test_exporter.py and the similar entries at the
other indicated spots) exceed the line-length rule (Ruff E501); fix by breaking
each long dict literal across multiple lines so each key/value pair is on its
own indented line (or wrap the value object across lines) and ensure trailing
commas and proper indentation are used so the dict remains valid and within the
line-length limit; apply the same wrapping style to the other offending dict
entries referenced in the comment.
- Line 495: Remove the unused local import of Path in the test module: delete
the "from pathlib import Path" import line in tests/test_exporter.py (the local
helper/function _make_exporter references no Path), ensuring no other code
depends on Path; run tests/lint to confirm Ruff F401 is resolved.
In `@tests/test_nb_dt_import.py`:
- Line 1472: The failing Ruff E501 errors come from overlong with-patch lines in
tests/test_nb_dt_import.py; break the single long context-manager line that
patches nb_dt_import.get_progress_panel returning _Ctx and core.export.Exporter
as MockExporter into multiple lines (one patch call per line or each argument on
its own line) and similarly wrap any long lambda expressions used in those tests
(also at the other occurrences referenced around lines 1527 and 1560) so each
line stays within the max length while keeping the same patched targets
(nb_dt_import.get_progress_panel, _Ctx, core.export.Exporter / MockExporter).
- Around line 1480-1481: Add a docstring to the public test class
TestMainAdditionalCoverage to satisfy Ruff D101: open the class declaration
containing the test method test_main_returns_early_for_export_diff and add a
short descriptive docstring (one or two sentences) under the class line
describing the purpose of these additional coverage tests.
- Around line 1395-1396: Add a brief class-level docstring to the public test
class TestDirectHelpers explaining its purpose (e.g., "Tests helper functions
for direct import behaviours such as handling missing pulse bar columns and
static empty bars for unknown totals"); update the class definition in
tests/test_nb_dt_import.py by inserting the docstring under class
TestDirectHelpers so Ruff D101 is satisfied.
In `@tests/test_netbox_api.py`:
- Line 7178: The assertion calling _delete_image_attachment is too long and
triggers E501; break the long assert expressions into multiple lines using
implicit line continuation with parentheses so each line stays within the max
length (e.g., wrap the function call arguments and move the "is True" or similar
comparator onto its own line). Apply the same wrapping style to the other long
asserts on the same test (the two other lines referenced) that use
_delete_image_attachment and mock_settings to ensure all three assertions
conform to line-length limits.
- Around line 7152-7153: Add a short triple-quoted docstring to each public test
class to satisfy D101: for TestNetBoxImageHelperFunctions add a one-line
docstring describing the purpose (e.g., "Tests for NetBox image helper
functions, including save_image_hash_cache behavior"), and do the same for the
other public test class referenced at line 7200 (replace with a concise one-line
description of that class's intent). Ensure the docstrings are placed
immediately below each class declaration (inside TestNetBoxImageHelperFunctions
and the other public class) and are written as standard Python triple-quoted
strings.
- Line 7106: Remove the unused import of _save_image_hash_cache from the test
file: delete the line importing _save_image_hash_cache (from core.netbox_api
import _save_image_hash_cache) in tests/test_netbox_api.py so the F401 lint
error is resolved; ensure no other references to _save_image_hash_cache exist in
the file before removing.
In `@tests/test_repo.py`:
- Around line 1270-1271: Add a one-line docstring to the public test class
TestRestrictedPickleLoading describing its purpose (e.g., "Tests that restricted
pickle loading rejects unsafe opcodes") to satisfy the D101 rule; place the
docstring immediately after the class declaration in the
TestRestrictedPickleLoading class definition.
---
Outside diff comments:
In `@core/export.py`:
- Around line 158-345: Exporter.run currently has too high cyclomatic complexity
(C901); refactor by extracting logical blocks into smaller helper methods to
reduce branching inside run. Split at least these sections into new private
methods on the same class: _collect_vendor_slugs_and_records (build
dt_by_vendor, mt_by_vendor, all_vendor_slugs), _compare_vendor_and_enqueue_items
(the per-vendor stale checks, _fetch_vendor_components and calls to
_determine_export_set_for_device_types/_module_types), _compare_rack_types
(rack_records loop and _determine_export_set_for_rack_types), and
_write_export_items (the write_task loop including _write_yaml,
_download_type_images and manifest updates). Keep run as an orchestrator that
calls these helpers in sequence and moves local variables (items, skipped_fresh,
manifest_path, progress) into arguments/returns as needed so each helper is
small and the overall cyclomatic complexity of run falls below the threshold.
In `@core/repo.py`:
- Around line 477-515: In resolve_slug_files(), validate pickle entry shapes and
constrain any joined paths to the repo root: when iterating known_slugs, ensure
each entry unpacks safely (e.g., check type and length before doing "for
entry_slug, relpath in known_slugs") and skip malformed entries; normalize
relpath and vendor_dir with os.path.normpath, then join with repo_root and
verify using os.path.commonpath (or equivalent) that the resulting abs_path
remains inside repo_root before appending to device_files; apply the same
shape-checking and path-normalization/containment checks for known_modules and
known_racks (when reading module_pickle and rack_pickle) and skip entries that
don't meet the expected tuple shape or that resolve outside the repository.
In `@nb-dt-import.py`:
- Around line 897-1173: The main() function is too large and triggers C901;
split it by extracting the vendor-processing loop and progress lifecycle into
one or two helper functions (e.g., create a function process_vendors(args,
settings, dtl_repo, netbox, handle) to encapsulate the for vendor in
vendors_to_process loop and its inner logic that calls _process_device_types,
_process_module_types, _process_rack_types, netbox.load_vendor,
netbox.create_manufacturers, and progress/task_registry management; and another
helper bootstrap_and_validate(args, settings, handle) to perform CLI validation,
arg normalization, NetBox/DTLRepo instantiation, _apply_slug_fast_path and
vendor discovery); ensure main() retains only high-level orchestration and calls
get_progress_panel and _finalize_task_registry as before, passing necessary
objects (dtl_repo, netbox, progress, task_registry) into the new helpers so
behavior and side-effects remain identical.
- Around line 867-877: The current fast-path makes the pickle index
authoritative and returns an empty narrowed list when matched_vendor_slugs is
empty; change the logic so that if matched_vendor_slugs is empty (i.e., no picks
from slug_resolved), treat it as a cache miss and fall back to the normal scan
by setting narrowed to vendors_to_process instead of [], and update the
verbose_log message accordingly; locate the block that computes
matched_vendor_slugs and narrowed (references: matched_vendor_slugs,
slug_resolved, vendors_to_process, narrowed, handle.verbose_log) and implement
the conditional: if not matched_vendor_slugs: narrowed = vendors_to_process (and
log a cache-miss message) else keep the current narrowing and log the resolved
counts.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34224db8-9142-4911-a74d-b183e592b627
📒 Files selected for processing (17)
.gitignorecore/export.pycore/export_manifest.pycore/nb_serializer.pycore/netbox_api.pycore/repo.pynb-dt-import.pytests/test_change_detector.pytests/test_export_manifest.pytests/test_exporter.pytests/test_nb_dt_import.pytests/test_nb_serializer.pytests/test_netbox_api.pytests/test_outcomes.pytests/test_repo.pytests/test_schema_reader.pytests/test_update_failure_resolver.py
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 `@core/export.py`:
- Line 3: The module docstring's entrypoint lists a constructor argument
`repo_path` that doesn't exist on Exporter.__init__; update the docstring to
match the actual signature used by Exporter.__init__ (remove `repo_path` or
replace with the correct parameter name), ensuring the documented call
Exporter(settings, handle, export_dir, force_overwrite, vendor_slugs).run() (or
the exact current params of Exporter.__init__) matches the code; locate the
docstring at the top of core/export.py and amend the entrypoint line to reflect
the real Exporter constructor parameters.
- Around line 370-377: The current merge of repo-only keys into to_write (in the
block using item.serialized, item.reason == "differs" and item.repo_yaml)
unintentionally preserves repo-only component lists (e.g., interfaces) and keeps
stale components; change the extra construction so it excludes component-style
keys: only copy repo_yaml keys that are not present in item.serialized and whose
values are not component lists (e.g., skip keys where item.repo_yaml[k] is a
list) or explicitly skip known component names (like "interfaces", "ports",
"vlans" etc. if you have a constant). In short, update the extra = {k: v for k,
v in item.repo_yaml.items() if k not in item.serialized} logic to also filter
out component/list keys before merging into to_write so only non-component
top-level repo-only fields are preserved.
In `@tests/test_netbox_api.py`:
- Around line 7230-7232: No code change required: the assignment
type(mock_pynetbox.api.return_value).version =
PropertyMock(side_effect=_requests.exceptions.ConnectionError("drop")) is safe
because mock_pynetbox is a function-scoped fixture and will be recreated per
test, so the PropertyMock on mock_pynetbox.api.return_value.version will not
leak into unrelated tests; leave the existing assignment as-is and only avoid
reassigning mock_pynetbox.api.return_value.version later in the same test to
prevent triggering the PropertyMock side effect.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98120ed9-5781-45fc-af7f-cbeea67c9f5d
📒 Files selected for processing (15)
core/export.pycore/nb_serializer.pycore/netbox_api.pycore/repo.pynb-dt-import.pytests/test_change_detector.pytests/test_export_manifest.pytests/test_exporter.pytests/test_graphql_client.pytests/test_nb_dt_import.pytests/test_nb_serializer.pytests/test_netbox_api.pytests/test_outcomes.pytests/test_repo.pytests/test_update_failure_resolver.py
💤 Files with no reviewable changes (3)
- tests/test_outcomes.py
- tests/test_update_failure_resolver.py
- core/repo.py
There was a problem hiding this comment.
Pull request overview
This PR refactors the import workflow to operate per vendor (rather than bulk), adds several new CLI capabilities (image verification, aggressive component cleanup, export-diff), and significantly improves Rich progress reporting for large multi-vendor runs. It also extends the GraphQL client to support vendor-scoped queries and adds new export-diff infrastructure (serializer, manifest, exporter).
Changes:
- Add per-vendor vendor discovery + vendor-scoped NetBox/GraphQL fetching and component preloading.
- Add new CLI flags and behaviors:
--verify-images,--remove-unmanaged-types, and--export-diff(+ manifest + exporter + serializer). - Improve progress UX/perf for multi-vendor runs (cumulative tasks, reduced refresh, no pulsing bars) and expand/adjust test coverage accordingly.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
nb-dt-import.py |
Main CLI refactor: per-vendor loop, new flags, progress/task registry, export-diff entry path, connection-error handling. |
core/repo.py |
Add vendor discovery and slug fast-path resolution via restricted pickle loading. |
core/graphql_client.py |
Add manufacturer-scoped filters, variables-based queries, component-template helper refactor, module image detail query, last_updated fields. |
core/netbox_api.py |
Per-vendor device-type loading, scoped component preload + integrity/count checks, image verification + hash cache, progress cleanup changes. |
core/change_detector.py |
Add remove_unmanaged_types behavior and suppress empty change-report banners. |
core/nb_serializer.py |
New NetBox→DTL serializer for export-diff output normalization and consistent omission of defaults. |
core/export.py |
New export-diff implementation (compare vs repo, manifest-based skipping, YAML writing, image downloading, vendor scoping). |
core/export_manifest.py |
New manifest helpers for incremental export-diff runs. |
README.md |
Document new CLI flags and image verification behavior. |
CHANGELOG.md |
Add feature note for --verify-images. |
.gitignore |
Ignore export artifacts and local caches. |
tests/test_nb_dt_import.py |
Update tests for per-vendor loop, new flag validation, progress/task helpers, export-diff parsing/behavior. |
tests/test_graphql_client.py |
Add tests for vendor-scoped filtering, variables usage, module image details, last_updated fields. |
tests/test_repo.py |
Add tests for vendor discovery and restricted pickle loading / slug fast path. |
tests/test_change_detector.py |
Add coverage for remove_unmanaged_types and new change-report logging behavior. |
tests/test_nb_serializer.py |
New test suite for serializer behavior and default omission. |
tests/test_export_manifest.py |
New test suite for export manifest helpers. |
tests/test_schema_reader.py |
Add validation test for schema properties type. |
tests/test_update_failure_resolver.py |
Add decode-failure edge case test for error payload extraction. |
tests/test_outcomes.py |
Add report rendering coverage for partial blockers + hint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marcinpsk
added a commit
that referenced
this pull request
May 23, 2026
- Use _build_auth_header() in _download_image() instead of hardcoded 'Token …' scheme so nbt_… tokens (Bearer) work correctly - Filter component-list keys from repo-only merge in differs write so NB stays authoritative for interfaces/ports/etc. - Broaden load_manifest() exception catch to OSError + ValueError (covers PermissionError, UnicodeDecodeError, etc.) - Guard att_id is a valid int before calling _delete_image_attachment in both 'missing' and 'hash-changed' verify-images paths - Skip module-YAML parsing when netbox.modules is False - Fix --verify-images help text and README to describe actual behavior (HTTP presence check + local hash cache; no remote hashing) - Fix _download_image return-type docstring to document _SKIP sentinel - Fix module docstring: remove non-existent repo_path constructor arg - Add tests for all new branches (att_id guard ×2, _is_subset sub-is-dict/sup-not-dict); 893 tests, 99.41% coverage
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 `@core/export.py`:
- Around line 755-760: The rename step in _download_image() can overwrite an
existing final filename because dest.rename(new_dest) doesn't check new_dest;
update the logic to honor self.force_overwrite by first checking if
new_dest.exists(), and if it exists and not self.force_overwrite then skip the
rename (return _SKIP or log and leave original), otherwise perform the rename
(or explicitly replace when force_overwrite is true). Reference
variables/functions: _download_image(), dest, new_dest, img_dir, new_name,
self.force_overwrite, _SKIP, and the existing handle.verbose_log call to report
when a rename is skipped or when an overwrite occurs.
In `@tests/test_exporter.py`:
- Around line 1205-1209: The test test_content_type_takes_priority_over_url is
too loose (it allows either URL-derived "front.jpg" or content-type-derived
"front.png"), so tighten it to assert that
_sanitize_attachment_filename("front", "/media/front.jpg", "image/png") returns
exactly the content-type-derived filename "front.png"; update the assertion in
that test to assert equality (result == "front.png") to catch regressions in
content-type precedence and keep fallback behavior covered by separate tests
that pass an empty content_type.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4300d0a-5b37-4fa0-b4c3-62acae7c5311
📒 Files selected for processing (9)
.markdownlint.json.pre-commit-config.yamlREADME.mdcore/export.pycore/export_manifest.pycore/netbox_api.pynb-dt-import.pytests/test_exporter.pytests/test_netbox_api.py
marcinpsk
added a commit
that referenced
this pull request
May 23, 2026
Fixes from CodeRabbit (major/minor): - core/export.py: _download_module_type_images rename step now respects force_overwrite — if new_dest already exists and force_overwrite=False, delete the provisional file instead of overwriting. Use dest.replace() for atomic rename when overwrite is allowed. Mark ok=False on rename OSError (was silently swallowed before). - tests/test_exporter.py: tighten test_content_type_takes_priority_over_url to assert result == 'front.png' exactly (content-type IS preferred over URL suffix in the current implementation). - tests/test_exporter.py: add test_download_module_type_images_rename_ skips_overwrite_when_dest_exists covering the new guard. Fixes from Copilot: - core/graphql_client.py: GraphQLCountMismatchError now subclasses GraphQLError so the top-level CLI except GraphQLError handler catches it and users see a clean error message instead of an unhandled traceback. - nb-dt-import.py: _validate_argument_combinations() now explicitly rejects --remove-unmanaged-types when --export-diff is set, giving a clear error message referencing --export-diff incompatibility. - tests/test_graphql_client.py: add TestErrorHierarchy with 2 tests confirming the inheritance relationship. - tests/test_nb_dt_import.py: add test covering the new --export-diff + --remove-unmanaged-types rejection.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/test_nb_serializer.py (1)
7-13: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace
MagicMockwith a real record stub.
MagicMockfabricates missing attributes, sogetattr(record, "...", None)never exercises the serializer's absent-field path and future field reads can silently turn into truthy mocks.Suggested test stub
-from unittest.mock import MagicMock +from types import SimpleNamespace ... def _dotdict(**kw): - """Build a simple MagicMock pretending to be a DotDict.""" - m = MagicMock() - for k, v in kw.items(): - setattr(m, k, v) - m.get = lambda key, default=None: kw.get(key, default) - return m + """Build a simple attribute container with DotDict-like `.get()` semantics.""" + class _Record(SimpleNamespace): + def get(self, key, default=None): + return getattr(self, key, default) + + return _Record(**kw)🤖 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 `@tests/test_nb_serializer.py` around lines 7 - 13, The test helper _dotdict currently uses MagicMock which fabricates missing attributes and prevents exercising absent-field logic; replace it with a lightweight real stub class (e.g., inside _dotdict) that assigns provided kwargs as attributes, implements get(self, key, default=None) to return the kw value or default, and does not create attributes that aren't set so getattr(record, "field", None) returns None for absent fields; update _dotdict to return an instance of that stub and remove reliance on MagicMock.core/nb_serializer.py (1)
166-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the legacy front-port shape as well.
NetBoxGraphQLClient.get_component_templates()has a pre-4.5 fallback that can returnrear_port_positiondirectly whenmappingsis unavailable, but this serializer only readsrecord.mappings. On older NetBox exports that means front-port linkage is silently dropped.Representative fix
mappings = getattr(record, "mappings", None) or [] if mappings: if len(mappings) > 1: ... m = mappings[0] rear_port = getattr(m, "rear_port", None) if rear_port: result["rear_port"] = rear_port.name rear_pos = getattr(m, "rear_port_position", None) rear_pos = _coerce_numeric(rear_pos) if rear_pos is not None and rear_pos > 1: result["rear_port_position"] = rear_pos + else: + rear_port = getattr(record, "rear_port", None) + if rear_port: + result["rear_port"] = getattr(rear_port, "name", rear_port) + rear_pos = _coerce_numeric(getattr(record, "rear_port_position", None)) + if rear_pos is not None and rear_pos > 1: + result["rear_port_position"] = rear_pos return result🤖 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 `@core/nb_serializer.py` around lines 166 - 184, The serializer only reads record.mappings and thus drops legacy front-port linkage returned by NetBoxGraphQLClient.get_component_templates() pre-4.5; update the logic in the block around mappings (the code that references mappings, m, rear_port, rear_port_position, _coerce_numeric, and result) so that if mappings is empty/None you also check for legacy attributes on record (e.g., record.rear_port and record.rear_port_position) and coerce/assign them to result the same way you do for mappings[0]; preserve the existing warning behavior for multiple mappings and reuse _coerce_numeric for numeric coercion.core/graphql_client.py (1)
18-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep count-mismatch failures out of the schema-fallback handlers.
GraphQLCountMismatchErroris documented as an abort condition, but once it inherits fromGraphQLError, the broadexcept GraphQLError:blocks later in this file will also catch it and treat truncation like an old-schema retry. That can let incomplete data continue through_query_component_endpoint(),get_module_type_images(), orget_module_type_image_details().Minimal mitigation pattern
- except GraphQLError: + except GraphQLCountMismatchError: + raise + except GraphQLError: ...Apply the same carve-out to each schema-compatibility fallback block.
🤖 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 `@core/graphql_client.py` around lines 18 - 23, GraphQLCountMismatchError (the class GraphQLCountMismatchError) must not be swallowed by the broad schema-fallback handlers that catch GraphQLError; update each except GraphQLError: block used in _query_component_endpoint, get_module_type_images, and get_module_type_image_details to immediately re-raise if the caught exception is a GraphQLCountMismatchError (e.g. except GraphQLError as e: if isinstance(e, GraphQLCountMismatchError): raise; else proceed with the existing fallback logic), so count-mismatch aborts are preserved while other GraphQLErrors keep the schema-compatibility retry behavior.core/export.py (1)
493-497:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse one canonical manufacturer key for module/rack repo matching.
These loaders can store repo YAML under slug-only keys like
("nokia", model), but Line 601 and Line 631 later only look up(rec.manufacturer.name, rec.model). Any repo file that usesmanufacturer: {slug: ...}is therefore treated asabsentand gets re-exported even though it already exists. Please normalize both load and lookup to the same manufacturer key, ideally the manufacturer slug, and add a regression that exercises slug-only manufacturer YAML end-to-end.Also applies to: 511-513
🤖 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 `@core/export.py` around lines 493 - 497, The manufacturer key used when loading repo YAML must be normalized to the canonical slug so lookups match; modify the loader that builds result[(mfr_name, data["model"])] to compute a single canonical mfr_slug (e.g., if mfr is a dict prefer mfr.get("slug") then fallback to mfr.get("name") or the raw string) and store keys as (mfr_slug, model), and update the lookup paths that currently use rec.manufacturer.name (used later when checking existence) to also resolve rec.manufacturer.slug (or normalize rec.manufacturer to the same mfr_slug) so both load and lookup use the slug; add a regression test that loads a repo YAML with manufacturer: {slug: "nokia"} and verifies it is treated as present (no duplicate export).
🤖 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 `@nb-dt-import.py`:
- Around line 1131-1134: The vendor and slug tokenization keeps leading/trailing
whitespace (and for vendors slugification runs on unstripped tokens), so update
the list comprehensions that build args.vendors and args.slugs to strip each
split token before further processing: for args.vendors trim the token (v)
before casefold()/re.sub() and for args.slugs trim the token (s) before adding
it to the list; locate the comprehensions that use the names vendor/v and slug/s
and apply .strip() to each split item prior to slugifying or appending.
---
Outside diff comments:
In `@core/export.py`:
- Around line 493-497: The manufacturer key used when loading repo YAML must be
normalized to the canonical slug so lookups match; modify the loader that builds
result[(mfr_name, data["model"])] to compute a single canonical mfr_slug (e.g.,
if mfr is a dict prefer mfr.get("slug") then fallback to mfr.get("name") or the
raw string) and store keys as (mfr_slug, model), and update the lookup paths
that currently use rec.manufacturer.name (used later when checking existence) to
also resolve rec.manufacturer.slug (or normalize rec.manufacturer to the same
mfr_slug) so both load and lookup use the slug; add a regression test that loads
a repo YAML with manufacturer: {slug: "nokia"} and verifies it is treated as
present (no duplicate export).
In `@core/graphql_client.py`:
- Around line 18-23: GraphQLCountMismatchError (the class
GraphQLCountMismatchError) must not be swallowed by the broad schema-fallback
handlers that catch GraphQLError; update each except GraphQLError: block used in
_query_component_endpoint, get_module_type_images, and
get_module_type_image_details to immediately re-raise if the caught exception is
a GraphQLCountMismatchError (e.g. except GraphQLError as e: if isinstance(e,
GraphQLCountMismatchError): raise; else proceed with the existing fallback
logic), so count-mismatch aborts are preserved while other GraphQLErrors keep
the schema-compatibility retry behavior.
In `@core/nb_serializer.py`:
- Around line 166-184: The serializer only reads record.mappings and thus drops
legacy front-port linkage returned by
NetBoxGraphQLClient.get_component_templates() pre-4.5; update the logic in the
block around mappings (the code that references mappings, m, rear_port,
rear_port_position, _coerce_numeric, and result) so that if mappings is
empty/None you also check for legacy attributes on record (e.g.,
record.rear_port and record.rear_port_position) and coerce/assign them to result
the same way you do for mappings[0]; preserve the existing warning behavior for
multiple mappings and reuse _coerce_numeric for numeric coercion.
In `@tests/test_nb_serializer.py`:
- Around line 7-13: The test helper _dotdict currently uses MagicMock which
fabricates missing attributes and prevents exercising absent-field logic;
replace it with a lightweight real stub class (e.g., inside _dotdict) that
assigns provided kwargs as attributes, implements get(self, key, default=None)
to return the kw value or default, and does not create attributes that aren't
set so getattr(record, "field", None) returns None for absent fields; update
_dotdict to return an instance of that stub and remove reliance on MagicMock.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: afb5493c-8f28-467f-bf96-4f5204c613c1
📒 Files selected for processing (8)
core/export.pycore/graphql_client.pycore/nb_serializer.pynb-dt-import.pytests/test_exporter.pytests/test_graphql_client.pytests/test_nb_dt_import.pytests/test_nb_serializer.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nb-dt-import.py (1)
850-875:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t auto-enable the pickle-backed slug fast path without an explicit trust gate.
This now turns any
--slugsrun into a repository-supplied pickle deserialization path. If that checkout or its cached index files are tampered with, the process can execute arbitrary code before normal YAML parsing ever starts. Please keep the safe scan as the default unless those pickle files are explicitly documented as fully trusted, or gate this optimization behind an opt-in until issue#75replaces pickle. Based on learnings: avoid using Pythonpickle.loadincore/modules unless the files are fully trusted, or migrate to a safer format as tracked by issue#75.🤖 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 `@nb-dt-import.py` around lines 850 - 875, The current _apply_slug_fast_path enables the pickle-backed fast path whenever args.slugs is set; change it to require an explicit trust gate (e.g., a new flag like args.trust_slug_pickle or args.use_pickle_index) before calling dtl_repo.resolve_slug_files so the safe full YAML scan remains the default; update the logic around args.slugs and dtl_repo.resolve_slug_files to check the new flag (and log via handle.verbose_log when the fast path is skipped or used) and ensure vendors_to_process is only narrowed when the opt-in flag is present and resolve_slug_files returns a non-None value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/export.py`:
- Around line 45-59: _repo_supersedes is falsely treating dict-form manufacturer
values as different because _canon_mfr_slug returns dict-provided slugs verbatim
and comparisons don't normalize both sides; update _canon_mfr_slug to always
produce a normalized lowercase slug (apply the same regex slugify to
mfr.get("slug") as to names and strings, lowercasing and stripping hyphens) and
then update _repo_supersedes to call _canon_mfr_slug on the repo manufacturer
value and slugify the serializer/plain-string manufacturer before comparing so
both sides are compared using the canonical slug form (use the existing
_canon_mfr_slug function name and the _repo_supersedes function name to locate
changes).
---
Outside diff comments:
In `@nb-dt-import.py`:
- Around line 850-875: The current _apply_slug_fast_path enables the
pickle-backed fast path whenever args.slugs is set; change it to require an
explicit trust gate (e.g., a new flag like args.trust_slug_pickle or
args.use_pickle_index) before calling dtl_repo.resolve_slug_files so the safe
full YAML scan remains the default; update the logic around args.slugs and
dtl_repo.resolve_slug_files to check the new flag (and log via
handle.verbose_log when the fast path is skipped or used) and ensure
vendors_to_process is only narrowed when the opt-in flag is present and
resolve_slug_files returns a non-None value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5da676a3-0cef-4e34-8ef3-0fb1a54b34dc
📒 Files selected for processing (6)
core/export.pycore/graphql_client.pycore/nb_serializer.pynb-dt-import.pytests/test_exporter.pytests/test_nb_serializer.py
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
nb-dt-import.py (1)
862-875:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat an empty slug fast-path result as a cache miss, not “no work”.
If
resolve_slug_files()can loadknown-slugs.picklebut the module/rack hint pickles are missing, unreadable, or just don't contain the requested slug,matched_vendor_slugsbecomes empty here. That collapsesvendors_to_processto[], and the run exits successfully without ever attempting the normal scan that would still find matching YAMLs. Please fall back to the full scan when the fast-path resolves zero vendors.💡 Minimal fix
matched_vendor_slugs = ( set(slug_resolved["device_files"]) | slug_resolved["module_vendors"] | slug_resolved["rack_vendors"] ) + if not matched_vendor_slugs: + handle.verbose_log("Slug pickle produced no vendor matches; falling back to full file scan.") + return vendors_to_process, None narrowed = [v for v in vendors_to_process if v["slug"] in matched_vendor_slugs]🤖 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 `@nb-dt-import.py` around lines 862 - 875, When slug_resolved = dtl_repo.resolve_slug_files(args.slugs) returns a non-None value but yields no matched vendors, treat that as a cache miss instead of “no work”: check if matched_vendor_slugs (computed from slug_resolved["device_files"], ["module_vendors"], ["rack_vendors"]) is empty and if so log the cache-miss and return vendors_to_process, None to force the full scan; otherwise continue returning narrowed, slug_resolved as before. Reference: resolve_slug_files, matched_vendor_slugs, vendors_to_process, narrowed.tests/test_exporter.py (1)
380-409: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a cross-vendor shared-slug regression here.
These cases only prove the new tuple key works for one manufacturer at a time. The production bug fixed in
core/export.pywas slug collision across vendors, so a flatslug -> yamlmap would still pass this suite. Please add one case with the same slug under two manufacturers and assert each NetBox record resolves against its own(mfr_slug, slug)entry.🤖 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 `@tests/test_exporter.py` around lines 380 - 409, Add a regression test that ensures slug collisions across manufacturers are resolved by the tuple key used by _determine_export_set_for_device_types: create two device types with the same slug but different manufacturer slugs, produce per-manufacturer repo YAMLs with serialize_device_type (or handcrafted repo_yaml), pass a repo_dt_by_slug mapping with keys like (manufacturer_slug, slug) for both entries, then call _determine_export_set_for_device_types and assert each NetBox record is matched against its corresponding (mfr_slug, slug) entry (i.e., one is not incorrectly matched to the other) and the export decision (no export or "differs") reflects the correct repo YAML for that manufacturer.
🤖 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 `@nb-dt-import.py`:
- Around line 862-875: When slug_resolved =
dtl_repo.resolve_slug_files(args.slugs) returns a non-None value but yields no
matched vendors, treat that as a cache miss instead of “no work”: check if
matched_vendor_slugs (computed from slug_resolved["device_files"],
["module_vendors"], ["rack_vendors"]) is empty and if so log the cache-miss and
return vendors_to_process, None to force the full scan; otherwise continue
returning narrowed, slug_resolved as before. Reference: resolve_slug_files,
matched_vendor_slugs, vendors_to_process, narrowed.
In `@tests/test_exporter.py`:
- Around line 380-409: Add a regression test that ensures slug collisions across
manufacturers are resolved by the tuple key used by
_determine_export_set_for_device_types: create two device types with the same
slug but different manufacturer slugs, produce per-manufacturer repo YAMLs with
serialize_device_type (or handcrafted repo_yaml), pass a repo_dt_by_slug mapping
with keys like (manufacturer_slug, slug) for both entries, then call
_determine_export_set_for_device_types and assert each NetBox record is matched
against its corresponding (mfr_slug, slug) entry (i.e., one is not incorrectly
matched to the other) and the export decision (no export or "differs") reflects
the correct repo YAML for that manufacturer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfb7291f-4e0d-4905-b858-1f78ce324795
📒 Files selected for processing (8)
.pre-commit-config.yamlcore/export.pycore/export_manifest.pycore/netbox_api.pycore/repo.pynb-dt-import.pytests/test_exporter.pytests/test_netbox_api.py
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
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 `@nb-dt-import.py`:
- Around line 1023-1032: The module and rack branches ignore the pre-resolved
slug hints and always glob/parse; update the logic around netbox.modules and
netbox.rack_types to first check vendor.get("slug_resolved") (or the local
slug_resolved structure) and, if it exists and indicates no candidate files for
modules or racks, set parsed_module_types or parsed_rack_types to []
immediately; only call dtl_repo.get_devices + dtl_repo.parse_files for modules
and _parse_vendor_racks for racks when slug_resolved is absent or indicates
there are candidate slugs to process (use the existing symbols netbox.modules,
dtl_repo.get_devices, dtl_repo.parse_files, _parse_vendor_racks,
parsed_module_types, parsed_rack_types and the vendor["name"]/args.slugs
checks).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61da4f92-ac17-4abb-97d2-ea037f62b9dc
📒 Files selected for processing (3)
core/export.pynb-dt-import.pytests/test_nb_dt_import.py
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 `@core/export.py`:
- Around line 500-512: The loader only scans "*.yaml" files so files with ".yml"
are missed; update _load_repo_device_types to also include ".yml" (e.g., iterate
both patterns or use a single rglob for "*.y*ml") when walking vdir.rglob, and
apply the same change to the sibling loader methods referenced in the comment
(the other functions around lines 514-527 and 529-542) so they also consider
both ".yaml" and ".yml" when calling yaml.safe_load and building their result
dictionaries.
- Around line 500-512: The loader currently lets duplicate (mfr_slug, slug) keys
silently last-win; make it deterministic and fail loudly by first collecting and
sorting the YAML file paths (e.g. list(vdir.rglob("*.yaml")) -> sorted(...))
and, as you process each file in _load_repo_device_types, check if the key
(mfr_slug, data["slug"]) already exists in result; if it does, raise an
informative exception (or call the existing error handler) that includes the
duplicate key and both file paths so upstream code can see the conflict. Apply
the same sorted-then-duplicate-detect pattern to the other similar loader
functions mentioned (the blocks at 514-527 and 529-542) to avoid
nondeterministic rglob ordering and silent last-wins.
- Around line 855-864: The current logic in export.py only compares netlocs
(base_host/target_host) before adding the Authorization header, which allows
credential leakage when the scheme differs (e.g., https base_url vs http
full_url); update the check to compare full origins (scheme + "://" + netloc)
derived from urlparse(self.base_url) and urlparse(full_url) instead of just
netloc so the token built by _build_auth_header(self.token) is only attached
when both scheme and host match exactly; adjust variable names (e.g.,
base_origin, target_origin) and keep the headers population using the existing
headers dict and _build_auth_header call.
- Around line 714-719: The image-existence check in core/export.py currently
uses a hardcoded exts tuple and can miss valid files (e.g.,
.webp/.bmp/.tiff/.svg); update the checks that build img paths (the img_dir /
f"{slug}.front.{e}" and img_dir / f"{slug}.rear.{e}" loops) to iterate over the
canonical IMAGE_EXTENSIONS list used by the downloader/sanitizer instead of the
local exts tuple, so both the front_url and rear_url conditions consult
IMAGE_EXTENSIONS when deciding to return "images-missing". Ensure
IMAGE_EXTENSIONS is imported/accessible in this module and replace the exts
reference with it in the any(...) checks.
In `@core/graphql_client.py`:
- Around line 366-393: The _build_manufacturer_filter function currently treats
any truthy slugs input as iterable (e.g. a plain string) which produces
malformed GraphQL variables; update _build_manufacturer_filter to first
normalize and validate slugs: if slugs is a str, convert to [slugs]; require
slugs to be a list/tuple of non-empty strings (strip whitespace and reject empty
strings), and raise a ValueError for invalid inputs; then proceed to return the
existing tuples using "manufacturer_slug"/"manufacturer_slugs" as before
(single-item -> $manufacturer_slug, multi-item -> $manufacturer_slugs).
In `@core/nb_serializer.py`:
- Around line 200-205: The sort key can return None and cause TypeError when
comparing to strings; change the key passed to sorted in the loop that calls
_serialize_front_port/_serialize_component so it normalizes missing or null
names to an empty string (e.g., use getattr(r, "name", None) and coalesce to ""
or use (getattr(r, "name", None) or "") ). Update the sorted(...) call that
currently uses key=lambda r: getattr(r, "name", "") so all records have string
keys and sorting never compares None with str.
In `@core/netbox_api.py`:
- Around line 394-397: The constructor currently calls _cache_dir.mkdir(...)
unconditionally which can raise and abort initialization; wrap the directory
creation and subsequent file-path setup in a try/except (catch OSError) so that
on failure you set self._image_hash_cache_path to None (or similar sentinel) and
self._image_hash_cache to an empty dict, and avoid calling
_load_image_hash_cache; keep using the same symbols (_cache_dir,
self._image_hash_cache_path, self._image_hash_cache, _load_image_hash_cache) so
downstream code can check for a None path and skip persisting the cache, and log
a non-fatal warning when falling back to the in-memory cache.
- Around line 177-183: The current _save_image_hash_cache(path, cache) silently
swallows exceptions which causes _is_image_hash_changed to treat missing entries
as unchanged; change _save_image_hash_cache to return a boolean success flag
(True on successful json.dump, False on exceptions), catch exceptions inside it
but do not suppress them silently—log or capture the exception message there and
return False; then update call sites that invoke _save_image_hash_cache to check
the returned bool and emit a single warning/log when it returns False so callers
know the cache wasn't persisted and reupload detection may be degraded.
In `@nb-dt-import.py`:
- Around line 1199-1223: After calling _apply_slug_fast_path(...) check whether
vendors_to_process is empty AND args.vendors (the explicit --vendors filter) was
provided; if so, fail fast with a non-zero exit and a clear error via the
existing CLI handle (e.g., log an error/fatal and exit) instead of proceeding to
get_progress_panel/_run_vendor_loop. Locate the check immediately after
vendors_to_process, slug_resolved = _apply_slug_fast_path(...) and before with
get_progress_panel(...); use the same handle object passed to this scope to
report the error so behavior is consistent with other pre-run guards.
In `@tests/test_nb_dt_import.py`:
- Around line 1397-1404: The tests currently only assert result.returncode != 0
which accepts any failure; update the subprocess.run assertions in
tests/test_nb_dt_import.py (the blocks invoking ["uv", "run", "--native-tls",
"nb-dt-import.py", "--export-diff", "--update"] and the two other similar
invocations around the same area) to assert the specific parser failure: check
result.returncode == 2 and assert that result.stderr contains the expected
parser error fragment (e.g. the mutual-exclusion or "argument --export-diff: not
allowed with argument --update" message) so the test verifies the parser
rejected the invalid flag combination rather than any arbitrary error.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05bbccac-68b4-483b-ac25-41b75f950abe
📒 Files selected for processing (25)
.gitignore.markdownlint.json.pre-commit-config.yamlCHANGELOG.mdREADME.mdcore/change_detector.pycore/export.pycore/export_manifest.pycore/graphql_client.pycore/nb_serializer.pycore/netbox_api.pycore/repo.pynb-dt-import.pytests/conftest.pytests/test_change_detector.pytests/test_export_manifest.pytests/test_exporter.pytests/test_graphql_client.pytests/test_nb_dt_import.pytests/test_nb_serializer.pytests/test_netbox_api.pytests/test_outcomes.pytests/test_repo.pytests/test_schema_reader.pytests/test_update_failure_resolver.py
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
nb-dt-import.py (1)
1023-1036:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMajor:
--slugscan silently skip all module/rack parsing when hint pickles are missing/unreadable.
core/repo.py::_vendor_slugs_from_pickle()returnsset()on any error (missing/unreadableknown-modules.pickle/known-racks.pickle).core/repo.py::resolve_slug_files()returnsNoneonly when the device pickle is unavailable; when it does returnslug_resolved, it provides"module_vendors"/"rack_vendors"as sets—so a hint-pickle failure collapses “unknown availability” into “definite no matches”, andnb-dt-import.py’s guards at the module/rack skip logic can forceparsed_module_types/parsed_rack_typesto[]instead of falling back to the full glob/parse path.Fix direction: make the module/rack hint result distinguishable from “no matches” (e.g., return
None/an explicit “hint unavailable” flag per pickle, and only apply the skip guard when the hint pickle load actually succeeded; otherwise fall back to normal parsing).🤖 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 `@nb-dt-import.py` around lines 1023 - 1036, The current logic treats a failed hint-pickle load as an empty set which makes _vendor_slugs_from_pickle()/resolve_slug_files() collapse “unknown” into “no matches”, causing nb-dt-import.py to set parsed_module_types and parsed_rack_types to [] and skip parsing; change the slug-hint API so resolve_slug_files() (or _vendor_slugs_from_pickle()) can return a distinct “hint unavailable” value (e.g., None vs empty set or an explicit flag) for module/rack pickles and then update the guards in nb-dt-import.py that check slug_resolved and vendor["slug"] (the branches that assign parsed_module_types and parsed_rack_types) to only skip parsing when the hint was successfully loaded and vendor is absent from module_vendors/rack_vendors; if the hint is unavailable, fall back to the normal module_files/dtl_repo.parse_files and _parse_vendor_racks paths instead of returning [].tests/test_nb_dt_import.py (3)
1759-1763:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe preload pump count is over-specified for this setup.
_run_vendor_loop()only wirespump_preload_progress()throughon_parse_step(), and in this test the three_process_*helpers are mocked out. The exactcall_count == 5assertion on Line 1761 therefore hard-codes behavior this test does not actually drive.Suggested fix
- # pump is now called after preload start, after create_manufacturers, - # and after each of the three process_*_types steps → 5 calls total - assert netbox.device_types.pump_preload_progress.call_count == 5 - netbox.device_types.pump_preload_progress.assert_called_with("job-1", progress) + netbox.device_types.pump_preload_progress.assert_called() + netbox.device_types.pump_preload_progress.assert_any_call("job-1", progress)🤖 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 `@tests/test_nb_dt_import.py` around lines 1759 - 1763, The test over-specifies pump_preload_progress call_count; locate the test assertions around netbox.device_types.pump_preload_progress in tests/test_nb_dt_import.py (related to _run_vendor_loop and on_parse_step) and remove or relax the exact call_count == 5 assertion since the three _process_* helpers are mocked and those calls are not exercised; instead keep/assert that pump_preload_progress was called with the expected args (use netbox.device_types.pump_preload_progress.assert_called_with("job-1", progress)) or simply assert it was called, and leave the existing stop_component_preload assertion intact.
1184-1188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the wrong preload path was never used.
The loop on Lines 1185-1188 only fails for unscoped
preload_all_components()calls, so this regression test would still pass if the flow accidentally switched topreload_all_components(manufacturer_slug=...). The contract here is stricter: this path should usestart_component_preload()and not callpreload_all_components()at all.Suggested fix
- # The unscoped preload_all_components (no manufacturer_slug) must NOT be called - for call in mock_nb.device_types.preload_all_components.call_args_list: - assert call.kwargs.get("manufacturer_slug") is not None, ( - "preload_all_components called without manufacturer_slug (global fetch triggered)" - ) + mock_nb.device_types.preload_all_components.assert_not_called()🤖 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 `@tests/test_nb_dt_import.py` around lines 1184 - 1188, Replace the current loop checking preload_all_components call args with a strict assertion that no calls were made to mock_nb.device_types.preload_all_components (i.e., assert mock_nb.device_types.preload_all_components.call_count == 0), and instead assert that mock_nb.device_types.start_component_preload was called with the expected manufacturer slug to verify the correct preload path; reference the mock object methods preload_all_components and start_component_preload to locate the test assertions.
1493-1537:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis test asserts a parser branch that doesn't exist and masks the
_run_export_diffcoverage below it.With the current args,
_validate_argument_combinations()reaches the existing--remove-unmanaged-types requires --remove-componentsbranch, not the export-diff-specific message asserted on Line 1508. That makes the test fail against the supplied implementation, and because the_run_export_diffblock is in the same test method, those assertions never execute either.🤖 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 `@tests/test_nb_dt_import.py` around lines 1493 - 1537, The test is asserting a non-existent parser branch so it fails and prevents the _run_export_diff assertions from running; update the first assertion in test_validate_argument_combinations_blocks_remove_unmanaged_with_export_diff to expect the actual message raised by _validate_argument_combinations ("--remove-unmanaged-types requires --remove-components") or change the input args to hit the export-diff-specific branch, and separate the _run_export_diff block into its own test (e.g., test_run_export_diff_calls_exporter) so the exporter assertions (patching get_progress_panel and core.export.Exporter, and checks on handle.set_console and MockExporter.return_value.run) always execute independently of the argument-validation branch. Ensure you reference the helper methods _validate_argument_combinations and _run_export_diff when relocating or adjusting the checks.
♻️ Duplicate comments (2)
core/export.py (1)
508-519:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet duplicate repo keys abort instead of being logged as malformed YAML.
The new duplicate-key
ValueErroris raised inside the same broadexcept Exceptionblock, so duplicate(manufacturer, slug/model)files are still silently skipped and the export continues with whichever file was seen first. That leaves export-diff comparing against an arbitrary repo source instead of failing loudly on ambiguous input.🔧 Minimal fix
for yaml_file in yaml_files: try: data = yaml.safe_load(yaml_file.read_text(encoding="utf-8")) - if isinstance(data, dict) and "slug" in data: - key = (mfr_slug, data["slug"]) - if key in result: - raise ValueError( - f"Duplicate repo device-type key {key!r}: {seen_files[key]} and {yaml_file}" - ) - result[key] = data - seen_files[key] = yaml_file except Exception as exc: self.handle.verbose_log(f"[yellow]Skipping malformed YAML {yaml_file}: {exc}[/yellow]") + continue + if isinstance(data, dict) and "slug" in data: + key = (mfr_slug, data["slug"]) + if key in result: + raise ValueError( + f"Duplicate repo device-type key {key!r}: {seen_files[key]} and {yaml_file}" + ) + result[key] = data + seen_files[key] = yaml_fileApply the same structure to the module-type and rack-type loaders.
Also applies to: 529-542, 552-565
🤖 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 `@core/export.py` around lines 508 - 519, The duplicate-key ValueError is being raised inside a broad except that treats it as a malformed YAML and silently skips duplicates; change the try/except around yaml.safe_load(...) in the block that populates result and seen_files so that only YAML/IO parsing errors are caught (e.g., catch yaml.YAMLError and OSError) and let ValueError propagate (or re-raise) so duplicates abort the run; specifically update the code around yaml.safe_load, result, seen_files and the except clause to only handle parsing/io exceptions, and apply the identical change to the module-type and rack-type loader blocks that have the same pattern.core/graphql_client.py (1)
381-395:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim validated manufacturer slugs before building GraphQL variables.
The validation uses
s.strip()only to reject blanks, but the original value is still sent to GraphQL. Inputs like["cisco "]pass validation and then filter on"cisco "exactly, which turns a valid vendor-scoped query into an empty result set.🔧 Minimal fix
if not isinstance(slugs, list) or any(not isinstance(s, str) or not s.strip() for s in slugs): raise ValueError("manufacturer_slugs must be None or a non-empty list of non-empty strings") + slugs = [s.strip() for s in slugs] if len(slugs) == 1: return ( ", $manufacturer_slug: String!",🤖 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 `@core/graphql_client.py` around lines 381 - 395, The code validates manufacturer slugs but does not trim them before building GraphQL variables, so inputs like "cisco " pass validation but are sent with trailing spaces; update the logic that handles slugs (the block returning the tuples and variables for manufacturer_slug / manufacturer_slugs) to strip each slug (e.g., map str.strip over slugs) after validation and before constructing the returned variables so the values in the dicts {"manufacturer_slug": ...} and {"manufacturer_slugs": ...} are trimmed; preserve the existing tuple shapes and error behavior for invalid inputs.
🤖 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 `@core/netbox_api.py`:
- Around line 177-190: _save_image_hash_cache currently overwrites path directly
which can leave a truncated file if the process dies; change it to write
atomically by writing the JSON to a temporary file in the same directory (use a
unique temp name), flush and fsync the temp file to ensure data hits disk, close
it, then atomically replace the target by os.replace or os.rename to move the
temp into path; ensure errors are caught and you clean up the temp file on
failure so callers of _save_image_hash_cache(path, cache) still get a reliable
True/False result.
---
Outside diff comments:
In `@nb-dt-import.py`:
- Around line 1023-1036: The current logic treats a failed hint-pickle load as
an empty set which makes _vendor_slugs_from_pickle()/resolve_slug_files()
collapse “unknown” into “no matches”, causing nb-dt-import.py to set
parsed_module_types and parsed_rack_types to [] and skip parsing; change the
slug-hint API so resolve_slug_files() (or _vendor_slugs_from_pickle()) can
return a distinct “hint unavailable” value (e.g., None vs empty set or an
explicit flag) for module/rack pickles and then update the guards in
nb-dt-import.py that check slug_resolved and vendor["slug"] (the branches that
assign parsed_module_types and parsed_rack_types) to only skip parsing when the
hint was successfully loaded and vendor is absent from
module_vendors/rack_vendors; if the hint is unavailable, fall back to the normal
module_files/dtl_repo.parse_files and _parse_vendor_racks paths instead of
returning [].
In `@tests/test_nb_dt_import.py`:
- Around line 1759-1763: The test over-specifies pump_preload_progress
call_count; locate the test assertions around
netbox.device_types.pump_preload_progress in tests/test_nb_dt_import.py (related
to _run_vendor_loop and on_parse_step) and remove or relax the exact call_count
== 5 assertion since the three _process_* helpers are mocked and those calls are
not exercised; instead keep/assert that pump_preload_progress was called with
the expected args (use
netbox.device_types.pump_preload_progress.assert_called_with("job-1", progress))
or simply assert it was called, and leave the existing stop_component_preload
assertion intact.
- Around line 1184-1188: Replace the current loop checking
preload_all_components call args with a strict assertion that no calls were made
to mock_nb.device_types.preload_all_components (i.e., assert
mock_nb.device_types.preload_all_components.call_count == 0), and instead assert
that mock_nb.device_types.start_component_preload was called with the expected
manufacturer slug to verify the correct preload path; reference the mock object
methods preload_all_components and start_component_preload to locate the test
assertions.
- Around line 1493-1537: The test is asserting a non-existent parser branch so
it fails and prevents the _run_export_diff assertions from running; update the
first assertion in
test_validate_argument_combinations_blocks_remove_unmanaged_with_export_diff to
expect the actual message raised by _validate_argument_combinations
("--remove-unmanaged-types requires --remove-components") or change the input
args to hit the export-diff-specific branch, and separate the _run_export_diff
block into its own test (e.g., test_run_export_diff_calls_exporter) so the
exporter assertions (patching get_progress_panel and core.export.Exporter, and
checks on handle.set_console and MockExporter.return_value.run) always execute
independently of the argument-validation branch. Ensure you reference the helper
methods _validate_argument_combinations and _run_export_diff when relocating or
adjusting the checks.
---
Duplicate comments:
In `@core/export.py`:
- Around line 508-519: The duplicate-key ValueError is being raised inside a
broad except that treats it as a malformed YAML and silently skips duplicates;
change the try/except around yaml.safe_load(...) in the block that populates
result and seen_files so that only YAML/IO parsing errors are caught (e.g.,
catch yaml.YAMLError and OSError) and let ValueError propagate (or re-raise) so
duplicates abort the run; specifically update the code around yaml.safe_load,
result, seen_files and the except clause to only handle parsing/io exceptions,
and apply the identical change to the module-type and rack-type loader blocks
that have the same pattern.
In `@core/graphql_client.py`:
- Around line 381-395: The code validates manufacturer slugs but does not trim
them before building GraphQL variables, so inputs like "cisco " pass validation
but are sent with trailing spaces; update the logic that handles slugs (the
block returning the tuples and variables for manufacturer_slug /
manufacturer_slugs) to strip each slug (e.g., map str.strip over slugs) after
validation and before constructing the returned variables so the values in the
dicts {"manufacturer_slug": ...} and {"manufacturer_slugs": ...} are trimmed;
preserve the existing tuple shapes and error behavior for invalid inputs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c2f225c-1bdf-45e2-8e93-aa0780eee1e7
📒 Files selected for processing (6)
core/export.pycore/graphql_client.pycore/nb_serializer.pycore/netbox_api.pynb-dt-import.pytests/test_nb_dt_import.py
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 `@tests/conftest.py`:
- Around line 6-13: The autouse fixture reset_graphql_clamping_warned imports
core.graphql_client before mock_env_vars runs; modify
reset_graphql_clamping_warned to depend on the mock_env_vars fixture (e.g., add
mock_env_vars as a parameter to the reset_graphql_clamping_warned function) so
pytest guarantees mock_env_vars runs first, then import core.graphql_client and
clear _CLAMPING_WARNED as before.
In `@tests/test_exporter.py`:
- Around line 483-512: There’s an orphan module-level docstring so the three
tests (test_writes_new_file, test_overwrite_guard_blocks_changed_file,
test_force_overwrite_allows_changed_file) are left inside
TestDetermineExportSet; fix by adding a new test class (e.g., class
TestYamlWriting:) immediately above the docstring so the docstring becomes that
class’s docstring and the three test functions become methods of TestYamlWriting
(ensure proper indentation and that they are not nested inside
TestDetermineExportSet).
In `@tests/test_nb_dt_import.py`:
- Around line 1493-1537: Split the single test into two isolated tests: keep the
first portion that constructs parser/args and asserts
nb_dt_import._validate_argument_combinations(parser, args) and
parser.error.assert_called_once_with(...) in the existing test named
test_validate_argument_combinations_blocks_remove_unmanaged_with_export_diff;
move the later setup (handle, progress, _Ctx, patching get_progress_panel and
core.export.Exporter, and the nb_dt_import._run_export_diff call plus
MockExporter/handle assertions) into a new test function (e.g.,
test_run_export_diff_wires_exporter_and_console) so the _run_export_diff wiring
is tested separately; preserve the same fixtures/patch usage and all assertions
(MockExporter.assert_called_once(),
handle.set_console.assert_called_once_with(progress.console),
MockExporter.return_value.run.assert_called_once_with(progress=progress).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c6eadfd-ddc5-4b54-b08a-22b39ba9fc05
📒 Files selected for processing (26)
.gitignore.markdownlint.json.pre-commit-config.yamlCHANGELOG.mdREADME.mdcore/change_detector.pycore/export.pycore/export_manifest.pycore/graphql_client.pycore/nb_serializer.pycore/netbox_api.pycore/repo.pyignored.jsonnb-dt-import.pytests/conftest.pytests/test_change_detector.pytests/test_export_manifest.pytests/test_exporter.pytests/test_graphql_client.pytests/test_nb_dt_import.pytests/test_nb_serializer.pytests/test_netbox_api.pytests/test_outcomes.pytests/test_repo.pytests/test_schema_reader.pytests/test_update_failure_resolver.py
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
nb-dt-import.py (1)
834-848:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unsupported filters in
--export-diffmode.
_run_export_diff()only forwardsexport_diff_dir,force_export_overwrite, andvendor_slugsintoExporter, so--slugs,--verify-images, and--force-resolve-conflictsare silently ignored here. The--slugscase is especially risky:--export-diff --slugs foocurrently exports every type for the selected vendors instead of the requested subset. Please fail fast on unsupported combinations instead of accepting a much broader run than the CLI implies.🔧 Minimal guard in the centralized validator
def _validate_argument_combinations(parser, args): """Apply mutual-dependency checks for CLI flags and exit via parser.error on violation.""" if args.export_diff and (args.update or args.only_new): parser.error("--export-diff cannot be used with --update or --only-new") if args.export_diff and args.remove_components: parser.error("--export-diff cannot be used with --remove-components") if args.export_diff and getattr(args, "remove_unmanaged_types", False): parser.error("--remove-unmanaged-types is an import-only flag and cannot be used with --export-diff") + if args.export_diff and args.slugs: + parser.error("--slugs is not supported with --export-diff") + if args.export_diff and args.verify_images: + parser.error("--verify-images is an import-only flag and cannot be used with --export-diff") + if args.export_diff and args.force_resolve_conflicts: + parser.error("--force-resolve-conflicts is an import-only flag and cannot be used with --export-diff") if args.remove_components and not args.update: parser.error("--remove-components requires --update") if args.remove_unmanaged_types and not args.remove_components: parser.error("--remove-unmanaged-types requires --remove-components") if args.force_resolve_conflicts and not args.update: parser.error("--force-resolve-conflicts requires --update")Also applies to: 883-893
🤖 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 `@nb-dt-import.py` around lines 834 - 848, In _validate_argument_combinations add explicit errors to reject flags that are unsupported when args.export_diff is set: check args.slugs (or args.slugs is not None / non-empty), args.verify_images, and the already-ignored args.force_resolve_conflicts and call parser.error with a clear message (e.g. "--export-diff cannot be used with --slugs/--verify-images/--force-resolve-conflicts") so _run_export_diff does not silently ignore those options; apply the same validation pattern for the other validator block referenced around _run_export_diff to ensure consistent fail-fast behavior.
🤖 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 `@nb-dt-import.py`:
- Around line 834-848: In _validate_argument_combinations add explicit errors to
reject flags that are unsupported when args.export_diff is set: check args.slugs
(or args.slugs is not None / non-empty), args.verify_images, and the
already-ignored args.force_resolve_conflicts and call parser.error with a clear
message (e.g. "--export-diff cannot be used with
--slugs/--verify-images/--force-resolve-conflicts") so _run_export_diff does not
silently ignore those options; apply the same validation pattern for the other
validator block referenced around _run_export_diff to ensure consistent
fail-fast behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d72cc8e7-ea0f-4ac8-999e-250a997fb76e
📒 Files selected for processing (1)
nb-dt-import.py
…types New flags: - --export-diff / --export-diff-dir / --force-export-overwrite: compare NetBox device types against a local DTL repo and export any that differ or are absent, writing them as YAML files - --verify-images: physically check images are present on the NetBox server (not just in DB) and re-upload if missing or changed - --remove-unmanaged-types: remove device/module/rack types from NetBox that are no longer present in the repo (requires --remove-components) - --slugs fast-path: resolve matching files via upstream pickle indexes before scanning, narrowing the vendor list for faster runs Core additions: - core/export.py: Exporter class with parallel component fetch, repo YAML comparison, order-insensitive diff, manufacturer slug normalisation, deterministic sorted YAML loading, duplicate-key detection, and --force-export-overwrite guard - core/export_manifest.py: manifest helpers for tracking exported files - core/nb_serializer.py: NetBox-to-DTL YAML serialiser with legacy front-port fallback and numeric-string coercion - core/graphql_client.py: last_updated field; get_module_type_image* endpoints; GraphQLCountMismatchError no longer swallowed; per-thread sessions closed after ThreadPoolExecutor; slug stripping - core/netbox_api.py: atomic image-hash cache writes (tempfile+fsync+ os.replace); _try_delete_stale_attachment helper; verify-images HTTP check with same-host auth guard; _fmt_connection_error helper - core/repo.py: _vendor_slugs_from_pickle returns None when unavailable (distinguishes 'no matches' from 'hint missing') - nb-dt-import.py: _parse_vendor_types / _parse_vendor_racks helpers; _apply_slug_fast_path with None-safe module/rack vendor hint guards; _validate_argument_combinations rejects import-only flags with --export-diff; per-vendor Vendors progress counter; NoPulseBarColumn; cumulative task registry; deduplicated GraphQL page-size warning Security / robustness: - Upstream pickle loads are guarded (security) - Image-hash cache migrated from pickle to JSON - Mixed str/float name lists sorted safely - UnicodeDecodeError caught in load_manifest - ConnectionError caught mid-run - YAML loader except narrowed to (yaml.YAMLError, OSError) so duplicate- key ValueError propagates Tests: 929 tests, all passing
marcinpsk
force-pushed
the
feat/per-vendor-fetch
branch
from
43c3292 to
b206eaa
Compare
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
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 by CodeRabbit
New Features
Documentation
Bug Fixes / UX
Tests
Chores