Repository navigation
fix(snmp-discovery): type standalone devices by their chassis part number - #665
Conversation
|
AI Code Review — risk tier: Advisory only. A human owns the merge decision. Summary — Net positive. This PR adds Fortinet model rewriting and D-Link family allow-listing to the chassis-model logic for SNMP discovery; the matching logic, guards, and defaults are correct and covered by tests, with no security-relevant concerns in the new code. No findings. 🤖 AI Code Review · run · prompts: Previous review · 2026-10-07 · 6304d2fAI Code Review — risk tier: Advisory only. A human owns the merge decision. Summary — Net-positive change. This is a re-review after three Codex-flagged edge cases (serial-less standalone rows, pinned-stack models with absent Resolved since last review
🤖 AI Code Review · run · prompts: Previous review · 2026-09-25 · 308aa6cAI Code Review — risk tier: Advisory only. A human owns the merge decision. Summary — Net-positive change. The chassis-model allow-list, pin-precedence rules, and OID-key normalization are well covered by tests, and both dimension reviews came back clean with no findings. 🤖 AI Code Review · run · prompts: |
|
Go test coverage
Total coverage: 91.3% |
Vulnerability Scan: PassedImage: No vulnerabilities found. Commit: 1e74c1d |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8482203f7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A standalone device kept the device type model from the sysObjectID lookup, a MIB product name, while every stack member already took its chassis row's entPhysicalModelName. The chassis row carries the part number a curated device type records, so the same switch model landed on different types depending on whether it was stacked, and a standalone device never matched a catalog type. A standalone device now takes its chassis model too, keeping the looked-up manufacturer. An operator-set device model still wins, and an empty, placeholder, unprintable or over-long value keeps the looked-up model. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Recorded walks show the chassis row's entPhysicalModelName is the orderable part number for Cisco, HP, Palo Alto Networks, Arista and HPE Aruba Networking, but a FRU number, OS version, chip or class name for many other vendors, which would be a worse type name than the lookup's. A standalone device now takes it only for those vendors, and not when the row survived only because others were refused. A model named in a lookup_extensions_dir entry now pins like one set in defaults or override_defaults, and a pinned model applies to a stack's master and members too, which previously took their chassis rows' models regardless. More firmware and class placeholders are refused. The backend docs describe the chassis row in the model precedence. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…only by defaults The docs seed lookup_extensions_dir by copying the bundled files, which flagged every copied entry as the operator's own, so the chassis model was never taken and pinned stacks collapsed onto the master's MIB name. A user entry now pins only when it changes or adds a model, and its key is normalised to the bundled spelling so an undotted key replaces the bundled entry instead of sitting beside it. A lookup entry pins a standalone device only; stack members keep their own chassis models as before. A defaults or override_defaults model pins the whole stack, when the mapper built a device type to carry it. HP is narrowed to the ProCurve and ArubaOS-Switch arc, the only one the recorded walks cover. The docs name the exact arcs, the stack rules and the precedence, and advise copying only the files you change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…talog only A user entry was compared with whatever the map held, earlier user files included, so the same model repeated in a second file stopped pinning, and a later file restoring the bundled model kept pinning it. Entries are now compared with a snapshot of the bundled catalog taken before any user file loads. When one file spells an OID both with and without the leading dot, the dotted entry wins deterministically and counts once. The runner reads the sysObjectID it already extracted instead of a second exported reader. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ey spelling A compile-time check keeps the production device lookup answering whether a model is the operator's, so renaming the method cannot silently drop the pin, and a test keeps every bundled key dotted, which the user-entry comparison relies on. The user flag's comment says what now sets it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t rules Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
8482203 to
ec35923
Compare
…orts no serial The inventory drops a chassis row with an empty serial, since serials identify stack members, so a standalone device whose one chassis row named its part but no serial kept the lookup model. With no inventory member, the sole chassis row now names the part under the same rules: one root row, an allow-listed vendor and no pinned model. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 575dc87a21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sysObjectID The mapper built a device type only from the sysObjectID lookup, so a model and manufacturer pinned in defaults went unused without one, and the stack pin, waiting on that type, let the chassis rows name the stack. Defaults setting both now build the type, and a defaults model pins a stack whether or not a type could be built. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4beb604b7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… when it has one The inventory drops a chassis row with no serial without counting it as refused, so a stack seen with one serial-less member looked standalone and took the other member's model. Both standalone paths now take the chassis model only when exactly one chassis row is reported. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/ai-review |
…lies by their chassis model Recorded FortiGate, FortiWiFi and FortiGate Rugged walks report the orderable model with underscores (FGT_60F); rewritten into Fortinet's form (FG-60F) they match catalog part numbers, where the lookup names fgt60F, or the 60F as fw60EI. Virtual FortiGates, which report their platform, and the 6000F, 7000E and 7000F chassis systems, whose one sysObjectID covers every chassis size, keep the lookup. The D-Link DGS families recorded report the model as sold, where the lookup has no entry and the raw sysObjectID was the model; other D-Link families, such as the DES-7200, are left out until recorded. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/ai-review |
|
🔁 AI Code Review updated for |
2 participants
Problem
SNMP discovery named a device's type in two different ways. A stack member took its chassis row's
entPhysicalModelName, which for most major vendors is the orderable part number (WS-C2960X-48FPD-L). A standalone device kept the name from the bundledsysObjectIDlookup, which is a MIB product name (catWsC2960x48fpdL). So the same hardware landed on two device types depending on whether it was stacked, and a standalone device never matched a curated device type such as those in the NetBox device-type library or NDX, which record the part number.Change
Standalone devices of trusted product lines take the chassis model. When the
sysObjectIDis under one of these arcs of1.3.6.1.4.1, the device type model comes from the single chassis row, keeping the looked-up manufacturer:9(Cisco)11.2.3.7(HP ProCurve and ArubaOS-Switch)25461(Palo Alto Networks)30065(Arista)47196(Aruba CX)12356.101.1(FortiGate, FortiWiFi and FortiGate Rugged), whose rows spell the model with underscores (FGT_60F). A Fortinet-only rewrite turns them into the orderable form (FG-60F,FGR-60F-3G4G). Virtual FortiGates (FGT_VM64,FGT_ARM64_…) and the 6000F, 7000E and 7000F chassis systems (.60001,.70001,.71201, where onesysObjectIDcovers every chassis size) keep the lookup name.171.10.70,.118,.119,.133,.137and.141(the D-Link DGS families recorded), which today have no lookup entry, so the rawsysObjectIDwas the model.Why an allow-list. Across the recorded standalone walks (144 with exactly one chassis row, out of 1,882 librenms and lab recordings), those arcs reported a part number there. The FortiGate and D-Link additions came from a survey of those recordings that three adversarial reviews checked: seven distinct FortiGate models across FortiOS 5.4 to 7.2 rewrite to exact catalog part numbers, and eight distinct D-Link switches report clean models. Other vendors report FRU numbers, OS versions, serial fragments, chip or class names, which would be a worse type name than the lookup's. They keep today's name, as do HP product lines outside ProCurve, Aruba wireless (
14823), Comware (25506) and Juniper, which has no recordings.Guards. The chassis model is not taken when it is empty, a placeholder (
N/A,Default string,chassisand similar), unprintable or longer than NetBox's 100 characters. It is taken only when the device reports exactly one chassis row, so not when other rows were refused as ambiguous or report no serial, and only when the device has a device type to carry it. A sole chassis row with no serial still names the model.Operator names win.
defaultsoroverride_defaultspins the whole target. That now includes a stack's master and every member, which previously took their own chassis models regardless; the old comment called this a contract bug.lookup_extensions_direntry that changes or adds a model pins a standalone device. Stack members keep their own chassis models under it, as they always have. An unchanged copy of a bundled entry pins nothing, so seeding the directory from the bundled files does not switch the change off. Entries are compared with the bundled catalog as loaded, before any user file.Docs. The backend docs describe the chassis row in the model precedence, name the exact arcs and the stack rules, and advise copying only the lookup files you change.
A device that answers no
sysObjectID. A model and manufacturer set together indefaultsoroverride_defaultsbecome its device type, and a stack pinned that way never takes its chassis rows' models.Left out. Juniper (no usable chassis rows in recorded walks), Huawei (empty rows), Dell Force10 (internal IDs), Extreme (truncated VOSS codes), Eltex (one wrong model), Dell OS10 (one recording) and vendors with a single walk wait for real captures.
Upgrade effects
sysObjectID) to the part number. Where a curated type carries that model, or that part number once the Diode plugin's part-number fallback ships (feat: bind a device type by part number when its model matches nothing, without writing to it diode-netbox-plugin#217), the device lands on it. Otherwise a new type named after the part number is created. The old types are left without devices; nothing is deleted.u_heightcan fail NetBox's rack validation until there is room.device.modelindefaultsoroverride_defaultsand are stacks now carry that model on every member.lookup_extensions_direntry for itssysObjectID, or setdevice.model.Not changed
Testing
override_defaults, manufacturer-only defaults, a user lookup entry, and a stack pinned both ways.go test ./...andgolangci-lint runpass inorb-discovery/snmp-discovery.🤖 Generated with Claude Code