carverauto/serviceradar

Fork 0

initial #2584

Merged

mfreeman451 merged 1 commit from refs/pull/2584/head into staging

2025-12-16 21:39:12 +00:00

mfreeman451 commented

2025-12-16 21:35:40 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2160
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2160
Original created: 2025-12-16T21:35:40Z
Original updated: 2025-12-16T21:39:15Z
Original head: carverauto/serviceradar:2154-snmp-octetstringobjectdescription-conversion-asserts-byte-instead-of-byte-causing-panic
Original base: staging
Original merged: 2025-12-16T21:39:12Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix, Tests

Description

Fix SNMP PDU string conversion panics for OctetString and ObjectDescription types
- Corrected type assertions from byte to []byte to match gosnmp library behavior
- Added proper error handling instead of panicking on unexpected value types
Add comprehensive regression tests for string type conversions
- Tests cover successful []byte conversion to string
- Tests verify error handling for unexpected value types without panicking
Document fix with specification and change proposal
- Added requirement specs and task tracking for the fix

Diagram Walkthrough

flowchart LR
  A["SNMP PDU with OctetString/ObjectDescription"] -->|"Previously: byte assertion"| B["PANIC: interface conversion error"]
  A -->|"Now: []byte assertion + error handling"| C["String conversion or error return"]
  D["Regression tests"] -->|"Verify correct behavior"| C
  D -->|"Verify no panics"| C

File Walkthrough

Relevant files

Bug fix

client.go

Fix byte string type assertions and error handling

pkg/checker/snmp/client.go

Removed ObjectDescription and OctetString from conversion map to
handle them explicitly
Added explicit type checks for ObjectDescription and OctetString
before map lookup
Fixed convertObjectDescription() to accept []byte instead of byte and
return error on type mismatch
Fixed convertOctetString() to accept []byte instead of byte and return
error on type mismatch
Both functions now return (interface{}, error) tuple for proper error
handling

+37/-21

Tests

client_conversion_test.go

Add SNMP string type conversion regression tests

pkg/checker/snmp/client_conversion_test.go

Added test for OctetString conversion with []byte value
Added test for ObjectDescription conversion with []byte value
Added parametrized test cases for unexpected value types that should
error without panicking
Tests verify both successful conversions and error handling paths

+95/-0

Documentation

proposal.md `Add change proposal documentation for fix` openspec/changes/fix-snmp-pdu-string-conversion/proposal.md Documents the bug: gosnmp returns `[]byte` but code asserts `byte`, causing panics Describes the fix: correct type assertions and add error handling Lists affected code and risk assessment	+17/-0
spec.md `Add specification requirements for string conversion` openspec/changes/fix-snmp-pdu-string-conversion/specs/snmp-checker/spec.md Defines requirement for SNMP PDU string types to convert without panics Specifies scenarios for successful `OctetString` and `ObjectDescription` conversion Specifies scenario for error handling with unexpected value types	+20/-0
tasks.md `Add task tracking for the fix implementation` openspec/changes/fix-snmp-pdu-string-conversion/tasks.md Documents completed tasks for fixing the conversion logic Lists completed test cases for regression coverage Tracks validation steps including tests, linting, and spec validation	+13/-0

Configuration changes

BUILD.bazel

Register new test file in build configuration

pkg/checker/snmp/BUILD.bazel

Added client_conversion_test.go to the test sources list

+1/-0

Imported from GitHub pull request. Original GitHub pull request: #2160 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2160 Original created: 2025-12-16T21:35:40Z Original updated: 2025-12-16T21:39:15Z Original head: carverauto/serviceradar:2154-snmp-octetstringobjectdescription-conversion-asserts-byte-instead-of-byte-causing-panic Original base: staging Original merged: 2025-12-16T21:39:12Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Fix SNMP PDU string conversion panics for `OctetString` and `ObjectDescription` types - Corrected type assertions from `byte` to `[]byte` to match gosnmp library behavior - Added proper error handling instead of panicking on unexpected value types - Add comprehensive regression tests for string type conversions - Tests cover successful `[]byte` conversion to string - Tests verify error handling for unexpected value types without panicking - Document fix with specification and change proposal - Added requirement specs and task tracking for the fix ___ ### Diagram Walkthrough ```mermaid flowchart LR A["SNMP PDU with OctetString/ObjectDescription"] -->|"Previously: byte assertion"| B["PANIC: interface conversion error"] A -->|"Now: []byte assertion + error handling"| C["String conversion or error return"] D["Regression tests"] -->|"Verify correct behavior"| C D -->|"Verify no panics"| C ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>client.go</strong><dd><code>Fix byte string type assertions and error handling</code>              </dd></summary> <hr> pkg/checker/snmp/client.go <ul><li>Removed <code>ObjectDescription</code> and <code>OctetString</code> from conversion map to <br>handle them explicitly<br> <li> Added explicit type checks for <code>ObjectDescription</code> and <code>OctetString</code> <br>before map lookup<br> <li> Fixed <code>convertObjectDescription()</code> to accept <code>[]byte</code> instead of <code>byte</code> and <br>return error on type mismatch<br> <li> Fixed <code>convertOctetString()</code> to accept <code>[]byte</code> instead of <code>byte</code> and return <br>error on type mismatch<br> <li> Both functions now return <code>(interface{}, error)</code> tuple for proper error <br>handling</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2160/files#diff-b5e92490b688495a040a2f6f5227dc83c46fc5e7ea59885f8285a3d6c868bd87">+37/-21</a>  </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>client_conversion_test.go</strong><dd><code>Add SNMP string type conversion regression tests</code>                  </dd></summary> <hr> pkg/checker/snmp/client_conversion_test.go <ul><li>Added test for <code>OctetString</code> conversion with <code>[]byte</code> value<br> <li> Added test for <code>ObjectDescription</code> conversion with <code>[]byte</code> value<br> <li> Added parametrized test cases for unexpected value types that should <br>error without panicking<br> <li> Tests verify both successful conversions and error handling paths</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2160/files#diff-e382a2bd9f2612005f0a6239ae6a30272af91d68a70e25b7ac3c0ed8da3211cc">+95/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>proposal.md</strong><dd><code>Add change proposal documentation for fix</code>                                </dd></summary> <hr> openspec/changes/fix-snmp-pdu-string-conversion/proposal.md <ul><li>Documents the bug: gosnmp returns <code>[]byte</code> but code asserts <code>byte</code>, <br>causing panics<br> <li> Describes the fix: correct type assertions and add error handling<br> <li> Lists affected code and risk assessment</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2160/files#diff-8bad22e013f99a913f7db91e78970c89634e94cd39262d339a06e72734531ee4">+17/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>spec.md</strong><dd><code>Add specification requirements for string conversion</code>          </dd></summary> <hr> openspec/changes/fix-snmp-pdu-string-conversion/specs/snmp-checker/spec.md <ul><li>Defines requirement for SNMP PDU string types to convert without <br>panics<br> <li> Specifies scenarios for successful <code>OctetString</code> and <code>ObjectDescription</code> <br>conversion<br> <li> Specifies scenario for error handling with unexpected value types</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2160/files#diff-fa51756b168cdca03f99da7568ffe261ea59b0b33f5c7ac1de4781af378f55fc">+20/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>Add task tracking for the fix implementation</code>                          </dd></summary> <hr> openspec/changes/fix-snmp-pdu-string-conversion/tasks.md <ul><li>Documents completed tasks for fixing the conversion logic<br> <li> Lists completed test cases for regression coverage<br> <li> Tracks validation steps including tests, linting, and spec validation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2160/files#diff-b18156d1b4a0904f964884f6642aae45fd88d0ddcab58b0c699d236e34be3cee">+13/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>BUILD.bazel</strong><dd><code>Register new test file in build configuration</code>                        </dd></summary> <hr> pkg/checker/snmp/BUILD.bazel - Added `client_conversion_test.go` to the test sources list </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2160/files#diff-bf78b9b2c6f1b0b501487be34f583e55b0735ad601abb7c7e9d27cb55a0ef57f">+1/-0</a>      </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-16 21:36:16 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2160#issuecomment-3662480215
Original created: 2025-12-16T21:36:16Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

🟢

No security concerns identified

No security vulnerabilities detected by AI analysis. Human verification advised for critical code.

Ticket Compliance

🟢

🎫 #2154

🟢	Fix SNMP PDU conversion for `OctetString` to assert `variable.Value` as `[]byte` (not `byte`) and convert to `string` without panicking.
	Fix SNMP PDU conversion for `ObjectDescription` to assert `variable.Value` as `[]byte` (not `byte`) and convert to `string` without panicking.
	Ensure unexpected `variable.Value` types for `OctetString`/`ObjectDescription` do not panic and instead return an error that callers can handle.
	Add regression tests covering `OctetString` and `ObjectDescription` conversions and verifying no-panics behavior on unexpected value types.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2160#issuecomment-3662480215 Original created: 2025-12-16T21:36:16Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟢</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2154>#2154</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Fix SNMP PDU conversion for <code>OctetString</code> to assert <code>variable.Value</code> as <code>[]byte</code> (not <code>byte</code>) and <br>convert to <code>string</code> without panicking.<br></td></tr> <tr><td>Fix SNMP PDU conversion for <code>ObjectDescription</code> to assert <code>variable.Value</code> as <code>[]byte</code> (not <br><code>byte</code>) and convert to <code>string</code> without panicking.<br></td></tr> <tr><td>Ensure unexpected <code>variable.Value</code> types for <code>OctetString</code>/<code>ObjectDescription</code> do not panic and <br>instead return an error that callers can handle.<br></td></tr> <tr><td>Add regression tests covering <code>OctetString</code> and <code>ObjectDescription</code> conversions and verifying <br>no-panics behavior on unexpected value types.<br></td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=6>🟢</td><td> <details><summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">   </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-12-16 21:37:45 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2160#issuecomment-3662484745
Original created: 2025-12-16T21:37:45Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	add panic recovery around conversions Add a panic recovery mechanism around calls to conversion functions in `conversionMap` to prevent crashes and return an error instead. pkg/checker/snmp/client.go [253-255] `if convertFunc, found := conversionMap[variable.Type]; found { - return convertFunc(variable), nil + // catch panics from type assertions in conversion functions + result, err := func() (_ interface{}, _ error) { + defer func() { + if r := recover(); r != nil { + _ = fmt.Errorf("%w: panic converting %v: %v", ErrSNMPConvert, variable.Type, r) + } + }() + return convertFunc(variable), nil + }() + return result, err }` Apply / Chat Suggestion importance[1-10]: 7 __ Why: This is a good defensive programming suggestion that improves the robustness of the conversion logic by catching potential panics from other conversion functions, which aligns with the PR's goal of preventing crashes.	Medium
General	factor common byte‐to‐string conversion Create a `convertBytesToString` helper function to consolidate the duplicated logic from `convertObjectDescription` and `convertOctetString`. pkg/checker/snmp/client.go [273-313] +func convertBytesToString(variable gosnmp.SnmpPDU, typeName string) (interface{}, error) { + b, ok := variable.Value.([]byte) + if !ok { + return nil, fmt.Errorf("%w: %s expected []byte, got %T", ErrSNMPConvert, typeName, variable.Value) + } + return string(b), nil +} + func convertObjectDescription(variable gosnmp.SnmpPDU) (interface{}, error) { - bytes, ok := variable.Value.([]byte) - if !ok { - return nil, fmt.Errorf("%w: ObjectDescription expected []byte, got %T", ErrSNMPConvert, variable.Value) - } - - return string(bytes), nil + return convertBytesToString(variable, "ObjectDescription") } func convertOctetString(variable gosnmp.SnmpPDU) (interface{}, error) { - bytes, ok := variable.Value.([]byte) - if !ok { - return nil, fmt.Errorf("%w: OctetString expected []byte, got %T", ErrSNMPConvert, variable.Value) - } - - return string(bytes), nil + return convertBytesToString(variable, "OctetString") } Apply / Chat Suggestion importance[1-10]: 6 __ Why: This is a good refactoring that reduces code duplication between `convertObjectDescription` and `convertOctetString` by extracting the common logic into a new helper function, improving maintainability.	Low
General	hoist conversion map to package scope Improve performance by moving the `conversionMap` definition out of the `convertVariable` function to a package-level variable, avoiding repeated allocations. pkg/checker/snmp/client.go [206-259] -func (SNMPClientImpl) convertVariable(variable gosnmp.SnmpPDU) (interface{}, error) { - // Map of SNMP types to conversion functions - conversionMap := map[gosnmp.Asn1BER]func(gosnmp.SnmpPDU) interface{}{ - gosnmp.Boolean: convertBoolean, - gosnmp.BitString: convertBitString, - // ... - } +var conversionMap = map[gosnmp.Asn1BER]func(gosnmp.SnmpPDU) interface{}{ + gosnmp.Boolean: convertBoolean, + gosnmp.BitString: convertBitString, // ... } +func (SNMPClientImpl) convertVariable(variable gosnmp.SnmpPDU) (interface{}, error) { + // use package‐level conversionMap without reallocating + // ... +} + `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 4 __ Why: This is a valid performance micro-optimization that avoids reallocating the `conversionMap` on each function call by defining it as a package-level variable.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2160#issuecomment-3662484745 Original created: 2025-12-16T21:37:45Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>add panic recovery around conversions</summary> ___ **Add a panic recovery mechanism around calls to conversion functions in <br><code>conversionMap</code> to prevent crashes and return an error instead.** [pkg/checker/snmp/client.go [253-255]](https://github.com/carverauto/serviceradar/pull/2160/files#diff-b5e92490b688495a040a2f6f5227dc83c46fc5e7ea59885f8285a3d6c868bd87R253-R255) ```diff if convertFunc, found := conversionMap[variable.Type]; found { - return convertFunc(variable), nil + // catch panics from type assertions in conversion functions + result, err := func() (_ interface{}, _ error) { + defer func() { + if r := recover(); r != nil { + _ = fmt.Errorf("%w: panic converting %v: %v", ErrSNMPConvert, variable.Type, r) + } + }() + return convertFunc(variable), nil + }() + return result, err } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: This is a good defensive programming suggestion that improves the robustness of the conversion logic by catching potential panics from other conversion functions, which aligns with the PR's goal of preventing crashes. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>factor common byte‐to‐string conversion</summary> ___ **Create a <code>convertBytesToString</code> helper function to consolidate the duplicated <br>logic from <code>convertObjectDescription</code> and <code>convertOctetString</code>.** [pkg/checker/snmp/client.go [273-313]](https://github.com/carverauto/serviceradar/pull/2160/files#diff-b5e92490b688495a040a2f6f5227dc83c46fc5e7ea59885f8285a3d6c868bd87R273-R313) ```diff +func convertBytesToString(variable gosnmp.SnmpPDU, typeName string) (interface{}, error) { + b, ok := variable.Value.([]byte) + if !ok { + return nil, fmt.Errorf("%w: %s expected []byte, got %T", ErrSNMPConvert, typeName, variable.Value) + } + return string(b), nil +} + func convertObjectDescription(variable gosnmp.SnmpPDU) (interface{}, error) { - bytes, ok := variable.Value.([]byte) - if !ok { - return nil, fmt.Errorf("%w: ObjectDescription expected []byte, got %T", ErrSNMPConvert, variable.Value) - } - - return string(bytes), nil + return convertBytesToString(variable, "ObjectDescription") } func convertOctetString(variable gosnmp.SnmpPDU) (interface{}, error) { - bytes, ok := variable.Value.([]byte) - if !ok { - return nil, fmt.Errorf("%w: OctetString expected []byte, got %T", ErrSNMPConvert, variable.Value) - } - - return string(bytes), nil + return convertBytesToString(variable, "OctetString") } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: This is a good refactoring that reduces code duplication between `convertObjectDescription` and `convertOctetString` by extracting the common logic into a new helper function, improving maintainability. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>hoist conversion map to package scope</summary> ___ **Improve performance by moving the <code>conversionMap</code> definition out of the <br><code>convertVariable</code> function to a package-level variable, avoiding repeated <br>allocations.** [pkg/checker/snmp/client.go [206-259]](https://github.com/carverauto/serviceradar/pull/2160/files#diff-b5e92490b688495a040a2f6f5227dc83c46fc5e7ea59885f8285a3d6c868bd87R206-R259) ```diff -func (*SNMPClientImpl) convertVariable(variable gosnmp.SnmpPDU) (interface{}, error) { - // Map of SNMP types to conversion functions - conversionMap := map[gosnmp.Asn1BER]func(gosnmp.SnmpPDU) interface{}{ - gosnmp.Boolean: convertBoolean, - gosnmp.BitString: convertBitString, - // ... - } +var conversionMap = map[gosnmp.Asn1BER]func(gosnmp.SnmpPDU) interface{}{ + gosnmp.Boolean: convertBoolean, + gosnmp.BitString: convertBitString, // ... } +func (*SNMPClientImpl) convertVariable(variable gosnmp.SnmpPDU) (interface{}, error) { + // use package‐level conversionMap without reallocating + // ... +} + ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: This is a valid performance micro-optimization that avoids reallocating the `conversionMap` on each function call by defining it as a package-level variable. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>