carverauto/serviceradar

Fork 0

2222 feat integrate snmp checker into agent #2669

Merged

mfreeman451 merged 9 commits from refs/pull/2669/head into staging

2026-01-14 17:56:04 +00:00

mfreeman451 commented

2026-01-14 17:26:03 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2298
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2298
Original created: 2026-01-14T17:26:03Z
Original updated: 2026-01-14T17:56:14Z
Original head: carverauto/serviceradar:2222-feat-integrate-snmp-checker-into-agent
Original base: staging
Original merged: 2026-01-14T17:56:04Z 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

Enhancement, Tests

Description

Integrates SNMP checker into the ServiceRadar agent with comprehensive configuration management and dynamic profile targeting
Implements SNMP profiles domain with four Ash resources (SNMPProfile, SNMPTarget, SNMPOIDConfig, SNMPOIDTemplate) supporting SNMPv1/v2c/v3 authentication
Adds SRQL-based device targeting for dynamic profile resolution with priority-based selection and default profile fallback
Provides 342 built-in OID templates organized by vendor (Standard, Cisco, Juniper, Arista) with template management and customization
Implements SNMPCompiler for transforming SNMP profiles into agent-consumable configuration with credential decryption
Adds embedded SNMP agent service with dynamic configuration loading, hot-reload capability, and status reporting
Extends protobuf definitions with SNMP configuration messages and enums for agent communication
Includes comprehensive LiveView UI for SNMP profiles management with CRUD operations, device targeting via query builder, and OID template browser
Adds encrypted credential storage for SNMP targets using AES-256-GCM encryption
Provides extensive test coverage including integration tests for config distribution, profile targeting, and agent service functionality
Includes database migration for SNMP infrastructure tables with proper foreign key relationships and constraints

Diagram Walkthrough

flowchart LR
  CP["Control Plane<br/>SNMP Profiles"] -->|Compile| SC["SNMPCompiler<br/>Config Generation"]
  SC -->|Proto Config| Agent["Agent Server<br/>SNMP Service"]
  Agent -->|Poll| Device["Network Devices<br/>via SNMP"]
  UI["Web UI<br/>SNMP LiveView"] -->|CRUD| CP
  CP -->|SRQL Query| TR["SrqlTargetResolver<br/>Device Targeting"]
  TR -->|Profile Selection| SC
  BT["Built-in OID<br/>Templates"] -->|Reference| UI
  Agent -->|Status| CP

File Walkthrough

Relevant files

Enhancement

22 files

index.ex `SNMP Profiles LiveView with targeting and OID management` web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex Implements comprehensive LiveView for SNMP profiles management with profile CRUD operations, device targeting via SRQL query builder, and OID template management Provides SNMP target configuration modal with support for SNMPv1/v2c/v3 authentication and connection testing via UDP reachability checks Includes OID management with template browser supporting both built-in and custom templates, with ability to copy and customize templates Implements query builder UI for SRQL-based interface targeting with real-time device count preview and filter management	+2953/-0
compiler.ex `Register SNMP compiler in agent config system` elixir/serviceradar_core/lib/serviceradar/agent_config/compiler.ex Adds `snmp` to the `config_type` type definition Registers `SNMPCompiler` in the compilers registry for SNMP configuration compilation	+3/-2
settings_components.ex `Add SNMP settings navigation menu item` web-ng/lib/serviceradar_web_ng_web/components/settings_components.ex Adds SNMP navigation link to settings sidebar menu Routes to `/settings/snmp` path with active state detection	+5/-0
builtin_templates.ex `Built-in SNMP OID templates for vendor-specific monitoring` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/builtin_templates.ex New module providing 342 lines of built-in OID templates organized by vendor (Standard, Cisco, Juniper, Arista) Implements template management functions: `all()`, `all_templates()`, `vendors()`, `for_vendor()`, and `seed!()` Includes pre-configured OID sets for common monitoring scenarios (system info, interfaces, CPU, memory, BGP, environment sensors) Provides stable ID generation for templates and database seeding with duplicate detection	+342/-0
snmp_profile.ex `SNMP profile resource with SRQL-based device targeting` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_profile.ex New Ash resource defining admin-managed SNMP monitoring profiles with multitenancy support Attributes include polling configuration (`poll_interval`, `timeout`, `retries`), device targeting via SRQL queries, and priority-based resolution Implements custom actions: `set_as_default`, `get_default`, `list_targeting_profiles` for profile management Enforces policies: admins can create/update, system actors bypass checks, default profile protection on deletion	+262/-0
snmp_target.ex `SNMP target configuration with multi-version authentication support` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_target.ex New Ash resource representing individual SNMP targets (network devices) within a profile Supports SNMPv1, v2c, and v3 authentication with encrypted credential storage via `EncryptCredentials` change Attributes include host, port, version, community string, and SNMPv3 parameters (username, security level, auth/priv protocols) Enforces unique target names per profile and role-based access control	+241/-0
snmp_oid_config.ex `OID configuration resource for SNMP data collection` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_oid_config.ex New Ash resource for individual OID configurations within SNMP targets Attributes define OID string, name, data type (counter, gauge, boolean, etc.), scale factor, and delta calculation flag Enforces unique OID per target and supports bulk creation via `create_bulk` action Implements role-based policies for admin-only modifications	+184/-0
snmp_oid_template.ex `Reusable OID template definitions with vendor categorization` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_oid_template.ex New Ash resource for reusable OID template definitions organized by vendor and category Supports built-in (read-only) and custom (user-created) templates with `is_builtin` flag Implements custom actions: `list_by_vendor`, `list_custom`, `list_builtin` for template filtering Prevents modification of built-in templates and enforces unique names per vendor/tenant	+213/-0
srql_target_resolver.ex `SRQL query evaluation for dynamic profile targeting` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/srql_target_resolver.ex New module implementing SRQL-based profile targeting resolution for SNMP profiles Validates device UIDs against UUID regex to prevent SRQL injection attacks Evaluates `target_query` fields on profiles to determine which profile applies to a device Supports both device and interface targeting with priority-based resolution (highest priority first)	+268/-0
snmp_compiler.ex `SNMP configuration compiler for agent consumption` elixir/serviceradar_core/lib/serviceradar/agent_config/compilers/snmp_compiler.ex New compiler module implementing `ServiceRadar.AgentConfig.Compiler` behaviour for SNMP configuration Transforms SNMPProfile resources into agent-consumable format with credential decryption and version formatting Implements profile resolution order: SRQL targeting profiles (by priority) then default profile fallback Provides `compile/4`, `validate/1`, `disabled_config/0`, and `resolve_profile/4` functions	+302/-0
validate_srql_query.ex `SRQL query validation change for profile targeting` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/validate_srql_query.ex New change module validating `target_query` attribute is valid SRQL syntax Normalizes queries by adding `in:devices` prefix if no `in:` prefix exists Supports both device and interface targeting queries Passes validation if query is nil/empty (no targeting behavior)	+62/-0
set_as_default.ex `Default profile enforcement change for SNMP profiles` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/set_as_default.ex New change module ensuring only one default SNMP profile exists per tenant Automatically unsets existing default profiles before setting new one via `unset_default` action Uses SystemActor for authorization-bypassing operations Extracts tenant ID from schema string format (`tenant_`)	+64/-0
encrypt_credentials.ex `SNMP credential encryption change for Ash resources` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/encrypt_credentials.ex New Ash resource change module that encrypts SNMP credentials before storage using Cloak/AES-256-GCM Intercepts plaintext `community`, `auth_password`, and `priv_password` arguments and encrypts them to `_encrypted` fields Implements write-only credential pattern where encrypted values cannot be read back in plaintext Adds error handling for encryption failures with user-friendly error messages	+51/-0
encrypt_passwords.ex `SNMPv3 password encryption change module` elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/encrypt_passwords.ex New Ash resource change module for encrypting SNMPv3 authentication and privacy passwords Converts plaintext `auth_password` and `priv_password` virtual attributes to encrypted storage fields Handles empty passwords by clearing encrypted fields and provides error handling for encryption failures Follows same write-only pattern as credential encryption	+47/-0
snmp_profiles.ex `SNMP profiles domain with resource registration` elixir/serviceradar_core/lib/serviceradar/snmp_profiles.ex New Ash domain module for SNMP monitoring configuration management Registers four SNMP-related resources: `SNMPProfile`, `SNMPTarget`, `SNMPOIDConfig`, `SNMPOIDTemplate` Enables AshAdmin for admin UI access and configures authorization with default authorization enabled Provides documentation on device targeting via SRQL queries similar to SysmonProfiles	+45/-0
router.ex `SNMP profiles routes in web router` web-ng/lib/serviceradar_web_ng_web/router.ex Added three new routes for SNMP profiles configuration in settings Routes map to `Settings.SNMPProfilesLive.Index` with actions for index, new profile, and edit profile Follows same pattern as existing sysmon profile routes	+5/-0
monitoring.pb.go `Protobuf SNMP configuration messages and enums` proto/monitoring.pb.go Added five new enum types for SNMP protocol support: `SNMPVersion`, `SNMPSecurityLevel`, `SNMPAuthProtocol`, `SNMPPrivProtocol`, `SNMPDataType` Added four new message types: `SNMPConfig`, `SNMPTargetConfig`, `SNMPv3Auth`, `SNMPOIDConfig` with full protobuf marshaling support Extended `AgentConfigResponse` message with new `snmp_config` field to include SNMP configuration Updated enum and message type indices and dependency mappings to accommodate new types	+821/-65
snmp_service.go `Embedded SNMP agent service with dynamic configuration` pkg/agent/snmp_service.go New 724-line service implementing embedded SNMP monitoring in the agent with config management Supports dynamic configuration loading from local files, cache, or control plane via `ApplyProtoConfig` Implements periodic config refresh loop with jitter and hot-reload capability Provides status reporting, platform-specific config paths, and factory pattern for testability	+724/-0
server.go `SNMP service integration into agent server` pkg/agent/server.go Added `initSNMPService` method to initialize and start embedded SNMP service during server startup Added `GetSNMPStatus` method to retrieve current SNMP status if service is running and enabled Added SNMP service shutdown logic in `Stop` method with error handling Integrated SNMP service initialization into `NewServer` constructor with graceful error handling	+47/-0
config.go `SNMP config loading and agent validation` pkg/checker/snmp/config.go Added `Enabled` field to `SNMPConfig` struct for controlling SNMP collection Implemented `DefaultConfig` function returning disabled SNMP configuration Implemented `LoadConfigFromFile` function to load SNMP config from JSON files Implemented `ValidateForAgent` method with less strict validation for agent use (no NodeAddress/ListenAddr requirement)	+62/-0
service.go `SNMP service factory for agent embedding` pkg/checker/snmp/service.go Added `NewSNMPServiceForAgent` function for creating SNMP services with agent-specific validation Extracted common service initialization into `newSNMPServiceInternal` helper function Enables SNMP service to be embedded in agent with less strict configuration requirements	+16/-1
types.go `SNMP service field in agent server type` pkg/agent/types.go Added `snmpService` field of type `*SNMPAgentService` to `Server` struct	+1/-0

Tests

7 files

snmp_config_distribution_integration_test.exs `SNMP config distribution integration tests` elixir/serviceradar_core/test/serviceradar/edge/snmp_config_distribution_integration_test.exs Adds integration tests for SNMP configuration distribution from control plane to agent Tests SNMP profile compilation, SRQL-based device targeting, and agent config generation Validates SNMPv1/v2c and SNMPv3 configuration compilation with proper credential handling Tests disabled profile handling and full agent config payload generation	+438/-0
snmp_profile_test.exs `SNMPProfile resource tests with CRUD and policy validation` elixir/serviceradar_core/test/serviceradar/snmp_profiles/snmp_profile_test.exs New test module with 246 lines covering SNMPProfile resource structure and CRUD operations Tests default values, action availability, and policy enforcement Integration tests verify profile creation, uniqueness constraints, and `set_as_default` behavior Uses test support utilities for tenant schema creation and cleanup	+246/-0
srql_target_resolver_test.exs `SrqlTargetResolver tests for SRQL-based profile targeting` elixir/serviceradar_core/test/serviceradar/snmp_profiles/srql_target_resolver_test.exs New test module with 375 lines testing SrqlTargetResolver profile matching logic Tests device UID validation, profile resolution with various SRQL queries, and priority-based selection Verifies disabled profile skipping, non-targeting profile filtering, and error handling Integration tests create devices and profiles to validate real query execution	+375/-0
snmp_compiler_test.exs `SNMPCompiler tests for config compilation and validation` elixir/serviceradar_core/test/serviceradar/snmp_profiles/snmp_compiler_test.exs New test module with 335 lines testing SNMPCompiler configuration compilation and validation Tests module structure, behaviour implementation, and disabled config generation Integration tests verify profile compilation with targets/OIDs, SNMPv3 credential handling, and profile resolution Validates output format matches proto SNMPConfig structure with proper credential decryption	+335/-0
snmp_integration_test.go `SNMP agent service integration tests` pkg/agent/snmp_integration_test.go Comprehensive integration test suite with 10 test cases covering SNMP agent service functionality Tests proto config application, enable/disable transitions, config updates, and concurrent operations Tests config file reloading, SNMPv3 configuration, status reporting, and error handling Uses mock service factory to avoid actual network SNMP polling during tests	+604/-0
testing.go `SNMP testing utilities and mock factories` pkg/checker/snmp/testing.go New testing utilities module with noop implementations of `Collector`, `Aggregator`, and their factories Provides `NewMockServiceForTesting` function to create SNMP services for agent tests without network requirements Enables testing of agent SNMP integration without actual SNMP device connections	+103/-0
checker_integration_test.go `ICMP checker integration test updates` pkg/agent/checker_integration_test.go Updated test to use new `NewICMPChecker` constructor with logger parameter Updated `Check` method call to include `proto.StatusRequest` parameter Updated response assertions to handle byte slice responses with string conversion Added proper error handling with `require.NoError` for checker creation	+14/-9

Configuration changes

4 files

20260114074955_add_snmp_profiles.exs `Database migration for SNMP profile infrastructure` elixir/serviceradar_core/priv/repo/tenant_migrations/20260114074955_add_snmp_profiles.exs New migration creating SNMP-related tables: `snmp_profiles`, `snmp_targets`, `snmp_oid_configs`, `snmp_oid_templates` Also creates `sysmon_profiles` table for system monitoring and adds `config_source` column to `ocsf_agents` Establishes foreign key relationships with cascading deletes and unique constraints per tenant Uses UUID v7 primary keys and encrypted binary columns for sensitive credentials	+229/-0
config.exs `Register SNMPProfiles domain in application configuration` web-ng/config/config.exs Adds `ServiceRadar.SNMPProfiles` domain to both web-ng and serviceradar_core domain registrations Maintains consistency with existing domain configuration pattern	+4/-2
config.exs `Register SNMP profiles domain in config` elixir/serviceradar_core/config/config.exs Added `ServiceRadar.SNMPProfiles` to the list of Ash domains in application configuration	+2/-1
test.exs `Register SNMP profiles domain in test config` elixir/serviceradar_core/config/test.exs Added `ServiceRadar.SNMPProfiles` to the list of Ash domains in test configuration	+1/-0

Formatting

3 files

index.ex `Minor formatting improvements in log live view` web-ng/lib/serviceradar_web_ng_web/live/log_live/index.ex Adds blank lines after Logger.warning calls in `duration_stats` and `sparklines` functions for formatting consistency	+2/-0
index.ex `Whitespace formatting adjustment in networks live view` web-ng/lib/serviceradar_web_ng_web/live/settings/networks_live/index.ex Minor whitespace formatting change in host status display text	+1/-1
index.ex `Blank line formatting in analytics live view` web-ng/lib/serviceradar_web_ng_web/live/analytics_live/index.ex Added blank line after logging statement for code formatting consistency	+1/-0

Documentation

2 files

sysmon_service.go

Documentation comment for sysmon service stop method

pkg/agent/sysmon_service.go

Added nolint comment documenting intentional parallel structure with
SNMPAgentService.Stop method

+2/-0

proposal.md

SNMP agent integration change proposal document

openspec/changes/integrate-snmp-agent/proposal.md

New change proposal document describing integration of SNMP checker
into the agent
Documents rationale, affected components (Go agent, Elixir core, UI,
Proto), and implementation approach
Outlines dynamic config loading, SRQL targeting, and local override
capabilities
Lists affected code locations and new resources being created

+42/-0

Additional files

13 files

snmp.md	+182/-0
20260114074956.json	+354/-0
20260114074956.json	+177/-0
20260114074956.json	+165/-0
20260114074956.json	+194/-0
20260114074956.json	+249/-0
20260114074956.json	+257/-0
design.md	+379/-0
spec.md	+120/-0
spec.md	+131/-0
tasks.md	+125/-0
snmp_service_test.go	+550/-0
monitoring.proto	+112/-0

Imported from GitHub pull request. Original GitHub pull request: #2298 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2298 Original created: 2026-01-14T17:26:03Z Original updated: 2026-01-14T17:56:14Z Original head: carverauto/serviceradar:2222-feat-integrate-snmp-checker-into-agent Original base: staging Original merged: 2026-01-14T17:56:04Z 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** Enhancement, Tests ___ ### **Description** - Integrates SNMP checker into the ServiceRadar agent with comprehensive configuration management and dynamic profile targeting - Implements SNMP profiles domain with four Ash resources (`SNMPProfile`, `SNMPTarget`, `SNMPOIDConfig`, `SNMPOIDTemplate`) supporting SNMPv1/v2c/v3 authentication - Adds SRQL-based device targeting for dynamic profile resolution with priority-based selection and default profile fallback - Provides 342 built-in OID templates organized by vendor (Standard, Cisco, Juniper, Arista) with template management and customization - Implements `SNMPCompiler` for transforming SNMP profiles into agent-consumable configuration with credential decryption - Adds embedded SNMP agent service with dynamic configuration loading, hot-reload capability, and status reporting - Extends protobuf definitions with SNMP configuration messages and enums for agent communication - Includes comprehensive LiveView UI for SNMP profiles management with CRUD operations, device targeting via query builder, and OID template browser - Adds encrypted credential storage for SNMP targets using AES-256-GCM encryption - Provides extensive test coverage including integration tests for config distribution, profile targeting, and agent service functionality - Includes database migration for SNMP infrastructure tables with proper foreign key relationships and constraints ___ ### Diagram Walkthrough ```mermaid flowchart LR CP["Control Plane SNMP Profiles"] -->|Compile| SC["SNMPCompiler Config Generation"] SC -->|Proto Config| Agent["Agent Server SNMP Service"] Agent -->|Poll| Device["Network Devices via SNMP"] UI["Web UI SNMP LiveView"] -->|CRUD| CP CP -->|SRQL Query| TR["SrqlTargetResolver Device Targeting"] TR -->|Profile Selection| SC BT["Built-in OID Templates"] -->|Reference| UI Agent -->|Status| CP ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><details><summary>22 files</summary><table> <tr> <td> <details> <summary>index.ex<dd><code>SNMP Profiles LiveView with targeting and OID management</code>  </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex <ul><li>Implements comprehensive LiveView for SNMP profiles management with profile CRUD operations, device targeting via SRQL query builder, and OID template management <li> Provides SNMP target configuration modal with support for SNMPv1/v2c/v3 authentication and connection testing via UDP reachability checks <li> Includes OID management with template browser supporting both built-in and custom templates, with ability to copy and customize templates <li> Implements query builder UI for SRQL-based interface targeting with real-time device count preview and filter management</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-1493b4100054d62c0d3fdc343e3fb7ce5f21735f752fad7d0fc978ae1ba114e4">+2953/-0</a></td> </tr> <tr> <td> <details> <summary>compiler.ex<dd><code>Register SNMP compiler in agent config system</code>                        </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/agent_config/compiler.ex <ul><li>Adds <code>snmp</code> to the <code>config_type</code> type definition <li> Registers <code>SNMPCompiler</code> in the compilers registry for SNMP configuration compilation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-c853b548702e07ffa78e0a371869cd58f00b8ec13836220866d34298e047cbcd">+3/-2</a>      </td> </tr> <tr> <td> <details> <summary>settings_components.ex<dd><code>Add SNMP settings navigation menu item</code>                                      </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/components/settings_components.ex <ul><li>Adds SNMP navigation link to settings sidebar menu <li> Routes to <code>/settings/snmp</code> path with active state detection</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-540f72cd405a3221e6f350afd04e644db83f0a504938a8907e94c417d070601e">+5/-0</a>      </td> </tr> <tr> <td> <details> <summary>builtin_templates.ex<dd><code>Built-in SNMP OID templates for vendor-specific monitoring</code></dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/builtin_templates.ex <ul><li>New module providing 342 lines of built-in OID templates organized by vendor (Standard, Cisco, Juniper, Arista) <li> Implements template management functions: <code>all()</code>, <code>all_templates()</code>, <code>vendors()</code>, <code>for_vendor()</code>, and <code>seed!()</code> <li> Includes pre-configured OID sets for common monitoring scenarios (system info, interfaces, CPU, memory, BGP, environment sensors) <li> Provides stable ID generation for templates and database seeding with duplicate detection</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-dfb09a552c8a813238fd1455326888586384c927450dedf18a30344548e3c683">+342/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_profile.ex<dd><code>SNMP profile resource with SRQL-based device targeting</code>      </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_profile.ex <ul><li>New Ash resource defining admin-managed SNMP monitoring profiles with multitenancy support <li> Attributes include polling configuration (<code>poll_interval</code>, <code>timeout</code>, <code>retries</code>), device targeting via SRQL queries, and priority-based resolution <li> Implements custom actions: <code>set_as_default</code>, <code>get_default</code>, <code>list_targeting_profiles</code> for profile management <li> Enforces policies: admins can create/update, system actors bypass checks, default profile protection on deletion</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-d829077b86a0e73a28a3b0ba483be2e13e79771c1c1c9661ddc33552441b476c">+262/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_target.ex<dd><code>SNMP target configuration with multi-version authentication support</code></dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_target.ex <ul><li>New Ash resource representing individual SNMP targets (network devices) within a profile <li> Supports SNMPv1, v2c, and v3 authentication with encrypted credential storage via <code>EncryptCredentials</code> change <li> Attributes include host, port, version, community string, and SNMPv3 parameters (username, security level, auth/priv protocols) <li> Enforces unique target names per profile and role-based access control</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-a7472ec491caa80d337ec0334013e0a936533d65cd3c84260ce5f6131d159d6d">+241/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_oid_config.ex<dd><code>OID configuration resource for SNMP data collection</code>            </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_oid_config.ex <ul><li>New Ash resource for individual OID configurations within SNMP targets <li> Attributes define OID string, name, data type (counter, gauge, boolean, etc.), scale factor, and delta calculation flag <li> Enforces unique OID per target and supports bulk creation via <code>create_bulk</code> action <li> Implements role-based policies for admin-only modifications</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-6aa0fe00d2c5f5ef6ec080fd9319ae4a3565d41709967832695930a133e23563">+184/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_oid_template.ex<dd><code>Reusable OID template definitions with vendor categorization</code></dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/snmp_oid_template.ex <ul><li>New Ash resource for reusable OID template definitions organized by vendor and category <li> Supports built-in (read-only) and custom (user-created) templates with <code>is_builtin</code> flag <li> Implements custom actions: <code>list_by_vendor</code>, <code>list_custom</code>, <code>list_builtin</code> for template filtering <li> Prevents modification of built-in templates and enforces unique names per vendor/tenant</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-265f44bb3b4d3c7da5baa3a8ddd402c87858edf8adcc831ac84468982a320d03">+213/-0</a>  </td> </tr> <tr> <td> <details> <summary>srql_target_resolver.ex<dd><code>SRQL query evaluation for dynamic profile targeting</code>            </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/srql_target_resolver.ex <ul><li>New module implementing SRQL-based profile targeting resolution for SNMP profiles <li> Validates device UIDs against UUID regex to prevent SRQL injection attacks <li> Evaluates <code>target_query</code> fields on profiles to determine which profile applies to a device <li> Supports both device and interface targeting with priority-based resolution (highest priority first)</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-2007e921214ed2cefc6170d60d4c2af9402cc499be1cd15737ec20344b2308ac">+268/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_compiler.ex<dd><code>SNMP configuration compiler for agent consumption</code>                </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/agent_config/compilers/snmp_compiler.ex <ul><li>New compiler module implementing <code>ServiceRadar.AgentConfig.Compiler</code> behaviour for SNMP configuration <li> Transforms SNMPProfile resources into agent-consumable format with credential decryption and version formatting <li> Implements profile resolution order: SRQL targeting profiles (by priority) then default profile fallback <li> Provides <code>compile/4</code>, <code>validate/1</code>, <code>disabled_config/0</code>, and <code>resolve_profile/4</code> functions</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-3f9551d39c5e77c379f701d5dee9c0a3d95e2a7861ba7515c402e10b585c61e0">+302/-0</a>  </td> </tr> <tr> <td> <details> <summary>validate_srql_query.ex<dd><code>SRQL query validation change for profile targeting</code>              </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/validate_srql_query.ex <ul><li>New change module validating <code>target_query</code> attribute is valid SRQL syntax <li> Normalizes queries by adding <code>in:devices</code> prefix if no <code>in:</code> prefix exists <li> Supports both device and interface targeting queries <li> Passes validation if query is nil/empty (no targeting behavior)</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-8eceb8f3bdc936bf2949b55d3f02daab69cbeb78ac93b94d982bd3840cab592c">+62/-0</a>    </td> </tr> <tr> <td> <details> <summary>set_as_default.ex<dd><code>Default profile enforcement change for SNMP profiles</code>          </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/set_as_default.ex <ul><li>New change module ensuring only one default SNMP profile exists per tenant <li> Automatically unsets existing default profiles before setting new one via <code>unset_default</code> action <li> Uses SystemActor for authorization-bypassing operations <li> Extracts tenant ID from schema string format (<code>tenant_<id></code>)</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-24900bdf51eca0e0319e2891a5ed38b0db74f9ff7dfcc316d8d0929d8306d95c">+64/-0</a>    </td> </tr> <tr> <td> <details> <summary>encrypt_credentials.ex<dd><code>SNMP credential encryption change for Ash resources</code>            </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/encrypt_credentials.ex <ul><li>New Ash resource change module that encrypts SNMP credentials before storage using Cloak/AES-256-GCM <li> Intercepts plaintext <code>community</code>, <code>auth_password</code>, and <code>priv_password</code> arguments and encrypts them to <code>_encrypted</code> fields <li> Implements write-only credential pattern where encrypted values cannot be read back in plaintext <li> Adds error handling for encryption failures with user-friendly error messages</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-bce52d49583a0ad577d67ade030aa382ced6f779a82f0f7fd489bdc8b3d5e6b3">+51/-0</a>    </td> </tr> <tr> <td> <details> <summary>encrypt_passwords.ex<dd><code>SNMPv3 password encryption change module</code>                                  </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/encrypt_passwords.ex <ul><li>New Ash resource change module for encrypting SNMPv3 authentication and privacy passwords <li> Converts plaintext <code>auth_password</code> and <code>priv_password</code> virtual attributes to encrypted storage fields <li> Handles empty passwords by clearing encrypted fields and provides error handling for encryption failures <li> Follows same write-only pattern as credential encryption</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-27d973f56e16c631b2556361857a11587a62d8c92a3e01b35c4590a1b524f4a8">+47/-0</a>    </td> </tr> <tr> <td> <details> <summary>snmp_profiles.ex<dd><code>SNMP profiles domain with resource registration</code>                    </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/snmp_profiles.ex <ul><li>New Ash domain module for SNMP monitoring configuration management <li> Registers four SNMP-related resources: <code>SNMPProfile</code>, <code>SNMPTarget</code>, <code>SNMPOIDConfig</code>, <code>SNMPOIDTemplate</code> <li> Enables AshAdmin for admin UI access and configures authorization with default authorization enabled <li> Provides documentation on device targeting via SRQL queries similar to SysmonProfiles</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-4abdb1bd929c42aab673ed4ca35faf8e5099df3b645d7b535121caf7a3a98fe3">+45/-0</a>    </td> </tr> <tr> <td> <details> <summary>router.ex<dd><code>SNMP profiles routes in web router</code>                                              </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/router.ex <ul><li>Added three new routes for SNMP profiles configuration in settings <li> Routes map to <code>Settings.SNMPProfilesLive.Index</code> with actions for index, new profile, and edit profile <li> Follows same pattern as existing sysmon profile routes</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-df516cd33165cd85914c1ccb3ff6511d3fe688d4a66498b99807958998c5d07c">+5/-0</a>      </td> </tr> <tr> <td> <details> <summary>monitoring.pb.go<dd><code>Protobuf SNMP configuration messages and enums</code>                      </dd></summary> <hr> proto/monitoring.pb.go <ul><li>Added five new enum types for SNMP protocol support: <code>SNMPVersion</code>, <code>SNMPSecurityLevel</code>, <code>SNMPAuthProtocol</code>, <code>SNMPPrivProtocol</code>, <code>SNMPDataType</code> <li> Added four new message types: <code>SNMPConfig</code>, <code>SNMPTargetConfig</code>, <code>SNMPv3Auth</code>, <code>SNMPOIDConfig</code> with full protobuf marshaling support <li> Extended <code>AgentConfigResponse</code> message with new <code>snmp_config</code> field to include SNMP configuration <li> Updated enum and message type indices and dependency mappings to accommodate new types</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-4f7e955b42854cc9cf3fb063b95e58a04f36271e6a0c1cb42ea6d7953dd96cc4">+821/-65</a></td> </tr> <tr> <td> <details> <summary>snmp_service.go<dd><code>Embedded SNMP agent service with dynamic configuration</code>      </dd></summary> <hr> pkg/agent/snmp_service.go <ul><li>New 724-line service implementing embedded SNMP monitoring in the agent with config management <li> Supports dynamic configuration loading from local files, cache, or control plane via <code>ApplyProtoConfig</code> <li> Implements periodic config refresh loop with jitter and hot-reload capability <li> Provides status reporting, platform-specific config paths, and factory pattern for testability</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-703d839c384ade98beb93a20adb32586e2035374ce27ca4861f7cdee656e1699">+724/-0</a>  </td> </tr> <tr> <td> <details> <summary>server.go<dd><code>SNMP service integration into agent server</code>                              </dd></summary> <hr> pkg/agent/server.go <ul><li>Added <code>initSNMPService</code> method to initialize and start embedded SNMP service during server startup <li> Added <code>GetSNMPStatus</code> method to retrieve current SNMP status if service is running and enabled <li> Added SNMP service shutdown logic in <code>Stop</code> method with error handling <li> Integrated SNMP service initialization into <code>NewServer</code> constructor with graceful error handling</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-3c51e5356a25859a3300ab62ed2494462feb2567ef69e6db3aa2bbc1032c1c5d">+47/-0</a>    </td> </tr> <tr> <td> <details> <summary>config.go<dd><code>SNMP config loading and agent validation</code>                                  </dd></summary> <hr> pkg/checker/snmp/config.go <ul><li>Added <code>Enabled</code> field to <code>SNMPConfig</code> struct for controlling SNMP collection <li> Implemented <code>DefaultConfig</code> function returning disabled SNMP configuration <li> Implemented <code>LoadConfigFromFile</code> function to load SNMP config from JSON files <li> Implemented <code>ValidateForAgent</code> method with less strict validation for agent use (no NodeAddress/ListenAddr requirement)</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-1e0bd78d2e0cae2b44ebfcbf0d0a4bedacd55d145b32e63150491b38a8da34cc">+62/-0</a>    </td> </tr> <tr> <td> <details> <summary>service.go<dd><code>SNMP service factory for agent embedding</code>                                  </dd></summary> <hr> pkg/checker/snmp/service.go <ul><li>Added <code>NewSNMPServiceForAgent</code> function for creating SNMP services with agent-specific validation <li> Extracted common service initialization into <code>newSNMPServiceInternal</code> helper function <li> Enables SNMP service to be embedded in agent with less strict configuration requirements</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-c592649b582c62a85d1dd416ac7fb609e55531cb43c8199a15f4432ce8ae05d8">+16/-1</a>    </td> </tr> <tr> <td> <details> <summary>types.go<dd><code>SNMP service field in agent server type</code>                                    </dd></summary> <hr> pkg/agent/types.go <ul><li>Added <code>snmpService</code> field of type <code>*SNMPAgentService</code> to <code>Server</code> struct</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-e3a3767c816cdb568a387a32243f7348046f1f3445549cc06368870c914496cd">+1/-0</a>      </td> </tr> </table></details></td></tr><tr><td>Tests</td><td><details><summary>7 files</summary><table> <tr> <td> <details> <summary>snmp_config_distribution_integration_test.exs<dd><code>SNMP config distribution integration tests</code>                              </dd></summary> <hr> elixir/serviceradar_core/test/serviceradar/edge/snmp_config_distribution_integration_test.exs <ul><li>Adds integration tests for SNMP configuration distribution from control plane to agent <li> Tests SNMP profile compilation, SRQL-based device targeting, and agent config generation <li> Validates SNMPv1/v2c and SNMPv3 configuration compilation with proper credential handling <li> Tests disabled profile handling and full agent config payload generation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-8fd152b39318fa1d1ea5d6e4e00cc87e9daee20c0b8af46c77d2b743d827961f">+438/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_profile_test.exs<dd><code>SNMPProfile resource tests with CRUD and policy validation</code></dd></summary> <hr> elixir/serviceradar_core/test/serviceradar/snmp_profiles/snmp_profile_test.exs <ul><li>New test module with 246 lines covering SNMPProfile resource structure and CRUD operations <li> Tests default values, action availability, and policy enforcement <li> Integration tests verify profile creation, uniqueness constraints, and <code>set_as_default</code> behavior <li> Uses test support utilities for tenant schema creation and cleanup</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-c1bc8c4b709ff8068058ec0cdacdb8c68443fc15d93bc8336c01aeca2f3d1664">+246/-0</a>  </td> </tr> <tr> <td> <details> <summary>srql_target_resolver_test.exs<dd><code>SrqlTargetResolver tests for SRQL-based profile targeting</code></dd></summary> <hr> elixir/serviceradar_core/test/serviceradar/snmp_profiles/srql_target_resolver_test.exs <ul><li>New test module with 375 lines testing SrqlTargetResolver profile matching logic <li> Tests device UID validation, profile resolution with various SRQL queries, and priority-based selection <li> Verifies disabled profile skipping, non-targeting profile filtering, and error handling <li> Integration tests create devices and profiles to validate real query execution</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-c2329efa90cd0fe64e211b4942fb3b43d549908829e53dd67c00303c355026e5">+375/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_compiler_test.exs<dd><code>SNMPCompiler tests for config compilation and validation</code>  </dd></summary> <hr> elixir/serviceradar_core/test/serviceradar/snmp_profiles/snmp_compiler_test.exs <ul><li>New test module with 335 lines testing SNMPCompiler configuration compilation and validation <li> Tests module structure, behaviour implementation, and disabled config generation <li> Integration tests verify profile compilation with targets/OIDs, SNMPv3 credential handling, and profile resolution <li> Validates output format matches proto SNMPConfig structure with proper credential decryption</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-00ebe1280516fdb784057290bda325a48925b8fd56219583641135addc9e6e69">+335/-0</a>  </td> </tr> <tr> <td> <details> <summary>snmp_integration_test.go<dd><code>SNMP agent service integration tests</code>                                          </dd></summary> <hr> pkg/agent/snmp_integration_test.go <ul><li>Comprehensive integration test suite with 10 test cases covering SNMP agent service functionality <li> Tests proto config application, enable/disable transitions, config updates, and concurrent operations <li> Tests config file reloading, SNMPv3 configuration, status reporting, and error handling <li> Uses mock service factory to avoid actual network SNMP polling during tests</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-f82d866873a075c9a17baa4d8e88054d4eb50a6f3a0e5eb1dd4a08e31cf35317">+604/-0</a>  </td> </tr> <tr> <td> <details> <summary>testing.go<dd><code>SNMP testing utilities and mock factories</code>                                </dd></summary> <hr> pkg/checker/snmp/testing.go <ul><li>New testing utilities module with noop implementations of <code>Collector</code>, <code>Aggregator</code>, and their factories <li> Provides <code>NewMockServiceForTesting</code> function to create SNMP services for agent tests without network requirements <li> Enables testing of agent SNMP integration without actual SNMP device connections</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-39f3d28a82022f03da84f91457d34822e658c61a7275ab1d040a4da5c341f315">+103/-0</a>  </td> </tr> <tr> <td> <details> <summary>checker_integration_test.go<dd><code>ICMP checker integration test updates</code>                                        </dd></summary> <hr> pkg/agent/checker_integration_test.go <ul><li>Updated test to use new <code>NewICMPChecker</code> constructor with logger parameter <li> Updated <code>Check</code> method call to include <code>proto.StatusRequest</code> parameter <li> Updated response assertions to handle byte slice responses with string conversion <li> Added proper error handling with <code>require.NoError</code> for checker creation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-ca90f1fec436a1e7e20474bc8c4f65d9217c83e94626cfefc2cb2f21151b3b8e">+14/-9</a>    </td> </tr> </table></details></td></tr><tr><td>Configuration changes</td><td><details><summary>4 files</summary><table> <tr> <td> <details> <summary>20260114074955_add_snmp_profiles.exs<dd><code>Database migration for SNMP profile infrastructure</code>              </dd></summary> <hr> elixir/serviceradar_core/priv/repo/tenant_migrations/20260114074955_add_snmp_profiles.exs <ul><li>New migration creating SNMP-related tables: <code>snmp_profiles</code>, <code>snmp_targets</code>, <code>snmp_oid_configs</code>, <code>snmp_oid_templates</code> <li> Also creates <code>sysmon_profiles</code> table for system monitoring and adds <code>config_source</code> column to <code>ocsf_agents</code> <li> Establishes foreign key relationships with cascading deletes and unique constraints per tenant <li> Uses UUID v7 primary keys and encrypted binary columns for sensitive credentials</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-4f3619b4ea9997d925d720374e4c4853c17ef9eacaa250690652550db711abd6">+229/-0</a>  </td> </tr> <tr> <td> <details> <summary>config.exs<dd><code>Register SNMPProfiles domain in application configuration</code></dd></summary> <hr> web-ng/config/config.exs <ul><li>Adds <code>ServiceRadar.SNMPProfiles</code> domain to both web-ng and serviceradar_core domain registrations <li> Maintains consistency with existing domain configuration pattern</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-da0b524f083d350207155c247526098fdd68866d64e7e4eae3d96fdae31bcfb6">+4/-2</a>      </td> </tr> <tr> <td> <details> <summary>config.exs<dd><code>Register SNMP profiles domain in config</code>                                    </dd></summary> <hr> elixir/serviceradar_core/config/config.exs <ul><li>Added <code>ServiceRadar.SNMPProfiles</code> to the list of Ash domains in application configuration</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-42b3888cc53d9dcf4ebc45ec7fffb2c672b129bffe763b6c76de58e4678a13a8">+2/-1</a>      </td> </tr> <tr> <td> <details> <summary>test.exs<dd><code>Register SNMP profiles domain in test config</code>                          </dd></summary> <hr> elixir/serviceradar_core/config/test.exs <ul><li>Added <code>ServiceRadar.SNMPProfiles</code> to the list of Ash domains in test configuration</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-d8fffec56a9bc7305c02bbdd424e21031ecbbb2c2e6ba07ba626e6043540e360">+1/-0</a>      </td> </tr> </table></details></td></tr><tr><td>Formatting</td><td><details><summary>3 files</summary><table> <tr> <td> <details> <summary>index.ex<dd><code>Minor formatting improvements in log live view</code>                      </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/live/log_live/index.ex <ul><li>Adds blank lines after Logger.warning calls in <code>duration_stats</code> and <code>sparklines</code> functions for formatting consistency</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-5c469994d989d04955c9b8a31536ed518a182b3f46819ecee9c3c22f651a4321">+2/-0</a>      </td> </tr> <tr> <td> <details> <summary>index.ex<dd><code>Whitespace formatting adjustment in networks live view</code>      </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/live/settings/networks_live/index.ex - Minor whitespace formatting change in host status display text </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4">+1/-1</a>      </td> </tr> <tr> <td> <details> <summary>index.ex<dd><code>Blank line formatting in analytics live view</code>                          </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/live/analytics_live/index.ex <ul><li>Added blank line after logging statement for code formatting consistency</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-35fb1715e171ac9f3240e9c02d2170b636acbb369952d58d352e1f75c91f82e3">+1/-0</a>      </td> </tr> </table></details></td></tr><tr><td>Documentation</td><td><details><summary>2 files</summary><table> <tr> <td> <details> <summary>sysmon_service.go<dd><code>Documentation comment for sysmon service stop method</code>          </dd></summary> <hr> pkg/agent/sysmon_service.go <ul><li>Added nolint comment documenting intentional parallel structure with <code>SNMPAgentService.Stop</code> method</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-8c00578b299aa029ef81f77ebe8959d20b7122299ee24e525c345f7c91c28d48">+2/-0</a>      </td> </tr> <tr> <td> <details> <summary>proposal.md<dd><code>SNMP agent integration change proposal document</code>                    </dd></summary> <hr> openspec/changes/integrate-snmp-agent/proposal.md <ul><li>New change proposal document describing integration of SNMP checker into the agent <li> Documents rationale, affected components (Go agent, Elixir core, UI, Proto), and implementation approach <li> Outlines dynamic config loading, SRQL targeting, and local override capabilities <li> Lists affected code locations and new resources being created</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-351f179fc846c318e3d803791ed050d95df43ebd5951bc2c38c9427eabb4ad15">+42/-0</a>    </td> </tr> </table></details></td></tr><tr><td>Additional files</td><td><details><summary>13 files</summary><table> <tr> <td>snmp.md</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-5d2a72888517a4c731287280253a29cedbcbe595f1ce57c2fa14aef37c62595a">+182/-0</a>  </td> </tr> <tr> <td>20260114074956.json</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-99b8acbc728a9a2e1fe4b7edd09b84052641f6159014142fbd55d85b9f03a7d8">+354/-0</a>  </td> </tr> <tr> <td>20260114074956.json</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-1c4f2389aded310f1ef3e57418269c50ea1f1e0e6a721b3646a0849a9a3ceddb">+177/-0</a>  </td> </tr> <tr> <td>20260114074956.json</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-7b96ff1dd5c08ade5b77b5cc473a54cc7615e998ebe905346a43dddf43389247">+165/-0</a>  </td> </tr> <tr> <td>20260114074956.json</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-533b7fce7ce1884234942bce26abaa886b0f18bee4f772f898539537232ca005">+194/-0</a>  </td> </tr> <tr> <td>20260114074956.json</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-16060141fd7139ea1c35574771d06bc31705d099ebd182e4e16ab86cebc16169">+249/-0</a>  </td> </tr> <tr> <td>20260114074956.json</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-179201ca3c8ff5fb9320b62893b5671f06a619f189bda00cc19aba8dcc252e48">+257/-0</a>  </td> </tr> <tr> <td>design.md</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-d5a94e78fcb56f7a87e5c6301357809709c351cbe0ee9b106d4faefd0347ecc7">+379/-0</a>  </td> </tr> <tr> <td>spec.md</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-4ae7d708fae44fe393b1908da2ffe7ebf1732536c6b21d6d39e01cc250679dbb">+120/-0</a>  </td> </tr> <tr> <td>spec.md</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-edc22549e1dd22326bd2c33d8865a117fd4d67c2a300ef36b9ee7d26a450ad04">+131/-0</a>  </td> </tr> <tr> <td>tasks.md</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-7bc05c95bc50ed240fcc70081ef9cca96b03ed5f74a4d1519173c82a59418171">+125/-0</a>  </td> </tr> <tr> <td>snmp_service_test.go</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-5286b0ba755cf551f524540ecf2f4926517f6f66748c25fc98ffe7363b917fe0">+550/-0</a>  </td> </tr> <tr> <td>monitoring.proto</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2298/files#diff-c4fc4e9693e4d2cf45697113ccca65b8c5ff18d2037e31ade411473533b36c2b">+112/-0</a>  </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-01-14 17:27:31 +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/2298#issuecomment-3750702808
Original created: 2026-01-14T17:27:31Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Sensitive config exposure

Description: The compiled SNMP config includes decrypted SNMPv2c community strings and SNMPv3 auth/priv
passwords (e.g., via decrypt_credential/1 and compile_v3_auth/1), which could be
exfiltrated if agent config payloads are logged, stored, or transmitted insecurely;
additionally the rescue logging inspect(e) risks leaking sensitive values if exceptions
contain decrypted data. snmp_compiler.ex [73-218]

Referred Code

def compile(tenant_id, _partition, agent_id, opts \\ []) do
  actor = opts[:actor] || SystemActor.for_tenant(tenant_id, :snmp_compiler)
  tenant_schema = TenantSchemas.schema_for_tenant(tenant_id)
  device_uid = opts[:device_uid]

  # Resolve the profile for this agent/device
  profile = resolve_profile(tenant_schema, device_uid, agent_id, actor)

  if profile do
    config = compile_profile(profile, tenant_schema, actor)
    {:ok, config}
  else
    # Return disabled config if no profile found
    {:ok, disabled_config()}
  end
rescue
  e ->
    Logger.error("SNMPCompiler: error compiling config - #{inspect(e)}")
    {:error, {:compilation_error, e}}
end



 ... (clipped 125 lines)

Ticket Compliance

🟡

🎫 #2222

🟢	Provide SNMP checking/config infrastructure that can be consumed by the agent (SNMP profiles, targets, OIDs, compilation, config distribution tests).
🔴	Eliminate the standalone `snmp-checker` deployment requirement and remove the agent→checker gRPC interaction.
⚪	Confirm the Go agent now embeds/implements the SNMP checker end-to-end (runtime behaviour), and that users no longer need to install/run the standalone `snmp-checker`.
⚪	Confirm any existing gRPC-based communication path to an external `snmp-checker` is removed/disabled and documentation/release notes reflect the new installation model.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

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:
Errors silently dropped: Several Ash read/decrypt failure paths return empty results or nil without surfacing
actionable context (e.g., decrypt_credential/1, load_profile_targets/3,
load_target_oids/3), which can mask production failures and make debugging difficult.

Referred Code

# Decrypt an encrypted credential, returning nil if not set
defp decrypt_credential(nil), do: nil

defp decrypt_credential(encrypted) do
  case Vault.decrypt(encrypted) do
    {:ok, decrypted} -> decrypted
    {:error, _} -> nil
  end
end

# Format version atom to string
defp format_version(:v1), do: "v1"
defp format_version(:v2c), do: "v2c"
defp format_version(:v3), do: "v3"
defp format_version(_), do: "v2c"

# Format security level atom to string
defp format_security_level(:no_auth_no_priv), do: "noAuthNoPriv"
defp format_security_level(:auth_no_priv), do: "authNoPriv"
defp format_security_level(:auth_priv), do: "authPriv"
defp format_security_level(_), do: "noAuthNoPriv"


 ... (clipped 45 lines)

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:
Exception returned outward: The compile/4 rescue returns {:error, {:compilation_error, e}} which can propagate
internal exception structures upstream instead of returning a generic user-facing error
while keeping details only in secure logs.

Referred Code

rescue
  e ->
    Logger.error("SNMPCompiler: error compiling config - #{inspect(e)}")
    {:error, {:compilation_error, e}}
end

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

⚪

Generic: Comprehensive Audit Trails

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

Status:
No audit logging: The new seed!/2 performs bulk creation of SNMP OID templates but does not emit any
explicit audit log entries indicating who seeded what and the outcome.

Referred Code

Seeds all built-in templates into the database for a tenant.
Skips templates that already exist.
"""
@spec seed!(String.t(), map()) :: {:ok, integer()} | {:error, term()}
def seed!(tenant_schema, actor) do
  templates = all()

  results =
    Enum.map(templates, fn template ->
      attrs = Map.put(template, :is_builtin, true)

      SNMPOIDTemplate
      |> Ash.Changeset.for_create(:create, attrs, actor: actor, tenant: tenant_schema)
      |> Ash.create(actor: actor)
      |> classify_seed_result()
    end)

  created = Enum.count(results, &(&1 == :created))
  {:ok, created}
end

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:
Logging raw exception: Logging inspect(e) in the SNMP compiler may emit sensitive internal details depending on
exception contents, and should be reviewed/structured to avoid leaking secrets in logs.

Referred Code

rescue
  e ->
    Logger.error("SNMPCompiler: error compiling config - #{inspect(e)}")
    {:error, {:compilation_error, e}}
end

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:
Partial SRQL validation: While device_uid is UUID-validated, the target_query content is executed via SRQL parsing
and filter application, so additional validation/authorization controls for user-provided
SRQL queries may be required but cannot be confirmed from this diff alone.

Referred Code

# Check if a profile's target_query matches a device
defp matches_device?(profile, tenant_schema, device_uid, actor) do
  target_query = profile.target_query

  if is_nil(target_query) or target_query == "" do
    {:ok, false}
  else
    # Determine if this is a device or interface query
    if String.starts_with?(target_query, "in:interfaces") do
      # Interface targeting - check if device has matching interfaces
      match_via_interfaces(target_query, tenant_schema, device_uid, actor)
    else
      # Device targeting - check if device matches directly
      combined_query = "#{target_query} uid:#{device_uid}"
      execute_device_match(combined_query, tenant_schema, actor)
    end
  end
end

# Execute a device match query
defp execute_device_match(query_string, tenant_schema, actor) do


 ... (clipped 48 lines)

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

Update

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/2298#issuecomment-3750702808 Original created: 2026-01-14T17:27:31Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary>Sensitive config exposure </summary> Description: The compiled SNMP config includes decrypted SNMPv2c community strings and SNMPv3 auth/priv passwords (e.g., via <code>decrypt_credential/1</code> and <code>compile_v3_auth/1</code>), which could be exfiltrated if agent config payloads are logged, stored, or transmitted insecurely; additionally the rescue logging <code>inspect(e)</code> risks leaking sensitive values if exceptions contain decrypted data. <a href='https://github.com/carverauto/serviceradar/pull/2298/files#diff-3f9551d39c5e77c379f701d5dee9c0a3d95e2a7861ba7515c402e10b585c61e0R73-R218'>snmp_compiler.ex [73-218]</a> <details open><summary>Referred Code</summary> ```elixir def compile(tenant_id, _partition, agent_id, opts \\ []) do actor = opts[:actor] || SystemActor.for_tenant(tenant_id, :snmp_compiler) tenant_schema = TenantSchemas.schema_for_tenant(tenant_id) device_uid = opts[:device_uid] # Resolve the profile for this agent/device profile = resolve_profile(tenant_schema, device_uid, agent_id, actor) if profile do config = compile_profile(profile, tenant_schema, actor) {:ok, config} else # Return disabled config if no profile found {:ok, disabled_config()} end rescue e -> Logger.error("SNMPCompiler: error compiling config - #{inspect(e)}") {:error, {:compilation_error, e}} end ... (clipped 125 lines) ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2222>#2222</a></summary> <table width='100%'><tbody> <tr><td rowspan=1>🟢</td> <td>Provide SNMP checking/config infrastructure that can be consumed by the agent (SNMP profiles, targets, OIDs, compilation, config distribution tests). </td></tr> <tr><td rowspan=1>🔴</td> <td>Eliminate the standalone <code>snmp-checker</code> deployment requirement and remove the agent→checker gRPC interaction. </td></tr> <tr><td rowspan=2>⚪</td> <td>Confirm the Go agent now embeds/implements the SNMP checker end-to-end (runtime behaviour), and that users no longer need to install/run the standalone <code>snmp-checker</code>. </td></tr> <tr><td>Confirm any existing gRPC-based communication path to an external <code>snmp-checker</code> is removed/disabled and documentation/release notes reflect the new installation model. </td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </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'>Custom Compliance</td></tr> <tr><td rowspan=1>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > 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 rowspan=2>🔴</td> <td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2298/files#diff-3f9551d39c5e77c379f701d5dee9c0a3d95e2a7861ba7515c402e10b585c61e0R211-R276'>Errors silently dropped</a>: Several Ash read/decrypt failure paths return empty results or nil without surfacing actionable context (e.g., <code>decrypt_credential/1</code>, <code>load_profile_targets/3</code>, <code>load_target_oids/3</code>), which can mask production failures and make debugging difficult. <details open><summary>Referred Code</summary> ```elixir # Decrypt an encrypted credential, returning nil if not set defp decrypt_credential(nil), do: nil defp decrypt_credential(encrypted) do case Vault.decrypt(encrypted) do {:ok, decrypted} -> decrypted {:error, _} -> nil end end # Format version atom to string defp format_version(:v1), do: "v1" defp format_version(:v2c), do: "v2c" defp format_version(:v3), do: "v3" defp format_version(_), do: "v2c" # Format security level atom to string defp format_security_level(:no_auth_no_priv), do: "noAuthNoPriv" defp format_security_level(:auth_no_priv), do: "authNoPriv" defp format_security_level(:auth_priv), do: "authPriv" defp format_security_level(_), do: "noAuthNoPriv" ... (clipped 45 lines) ``` </details> > 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>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2298/files#diff-3f9551d39c5e77c379f701d5dee9c0a3d95e2a7861ba7515c402e10b585c61e0R88-R92'>Exception returned outward</a>: The <code>compile/4</code> rescue returns <code>{:error, {:compilation_error, e}}</code> which can propagate internal exception structures upstream instead of returning a generic user-facing error while keeping details only in secure logs. <details open><summary>Referred Code</summary> ```elixir rescue e -> Logger.error("SNMPCompiler: error compiling config - #{inspect(e)}") {:error, {:compilation_error, e}} end ``` </details> > 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 rowspan=3>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2298/files#diff-dfb09a552c8a813238fd1455326888586384c927450dedf18a30344548e3c683R83-R102'>No audit logging</a>: The new <code>seed!/2</code> performs bulk creation of SNMP OID templates but does not emit any explicit audit log entries indicating who seeded what and the outcome. <details open><summary>Referred Code</summary> ```elixir Seeds all built-in templates into the database for a tenant. Skips templates that already exist. """ @spec seed!(String.t(), map()) :: {:ok, integer()} | {:error, term()} def seed!(tenant_schema, actor) do templates = all() results = Enum.map(templates, fn template -> attrs = Map.put(template, :is_builtin, true) SNMPOIDTemplate |> Ash.Changeset.for_create(:create, attrs, actor: actor, tenant: tenant_schema) |> Ash.create(actor: actor) |> classify_seed_result() end) created = Enum.count(results, &(&1 == :created)) {:ok, created} end ``` </details> > 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>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2298/files#diff-3f9551d39c5e77c379f701d5dee9c0a3d95e2a7861ba7515c402e10b585c61e0R88-R92'>Logging raw exception</a>: Logging <code>inspect(e)</code> in the SNMP compiler may emit sensitive internal details depending on exception contents, and should be reviewed/structured to avoid leaking secrets in logs. <details open><summary>Referred Code</summary> ```elixir rescue e -> Logger.error("SNMPCompiler: error compiling config - #{inspect(e)}") {:error, {:compilation_error, e}} end ``` </details> > 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>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2298/files#diff-2007e921214ed2cefc6170d60d4c2af9402cc499be1cd15737ec20344b2308acR121-R189'>Partial SRQL validation</a>: While <code>device_uid</code> is UUID-validated, the <code>target_query</code> content is executed via SRQL parsing and filter application, so additional validation/authorization controls for user-provided SRQL queries may be required but cannot be confirmed from this diff alone. <details open><summary>Referred Code</summary> ```elixir # Check if a profile's target_query matches a device defp matches_device?(profile, tenant_schema, device_uid, actor) do target_query = profile.target_query if is_nil(target_query) or target_query == "" do {:ok, false} else # Determine if this is a device or interface query if String.starts_with?(target_query, "in:interfaces") do # Interface targeting - check if device has matching interfaces match_via_interfaces(target_query, tenant_schema, device_uid, actor) else # Device targeting - check if device matches directly combined_query = "#{target_query} uid:#{device_uid}" execute_device_match(combined_query, tenant_schema, actor) end end end # Execute a device match query defp execute_device_match(query_string, tenant_schema, actor) do ... (clipped 48 lines) ``` </details> > 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"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2026-01-14 17:29:03 +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/2298#issuecomment-3750712054
Original created: 2026-01-14T17:29:03Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Prevent accidental deletion of credentials~~ Suggestion Impact: The commit adds logic in handle_event("save_target") to reject empty values for community/auth_password/priv_password when editing a target, preventing accidental clearing of stored encrypted credentials. (Other unrelated changes were also included.) code diff: @@ -460,6 +460,19 @@ scope = socket.assigns.current_scope profile_id = socket.assigns.selected_profile.id + # When editing, remove blank password/community fields from params + # to avoid accidentally clearing existing encrypted credentials. + params = + if socket.assigns.editing_target do + sensitive_fields = ["community", "auth_password", "priv_password"] + + Map.reject(params, fn {key, value} -> + key in sensitive_fields and value == "" + end) + else + params + end + target_form = if socket.assigns.editing_target do Form.for_update(socket.assigns.editing_target, :update, @@ -529,7 +542,13 @@ In the `save_target` event handler, when editing a target, remove blank password and community string fields from the submitted parameters to avoid unintentionally clearing existing credentials. web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [459-495] def handle_event("save_target", %{"form" => params}, socket) do scope = socket.assigns.current_scope profile_id = socket.assigns.selected_profile.id + + # When editing, remove blank password/community fields from params + # to avoid accidentally clearing them. + params = + if socket.assigns.editing_target do + params + \|> Map.delete_if(fn key, value -> + key in ["community", "auth_password", "priv_password"] and value == "" + end) + else + params + end target_form = if socket.assigns.editing_target do Form.for_update(socket.assigns.editing_target, :update, domain: ServiceRadar.SNMPProfiles, scope: scope ) else Form.for_create(SNMPTarget, :create, domain: ServiceRadar.SNMPProfiles, scope: scope, params: %{snmp_profile_id: profile_id} ) end target_form = Form.validate(target_form, params) case Form.submit(target_form, params: params) do {:ok, _target} -> action = if socket.assigns.editing_target, do: "updated", else: "created" targets = load_profile_targets(scope, profile_id) {:noreply, socket \|> assign(:targets, targets) \|> assign(:show_target_modal, false) \|> assign(:target_form, nil) \|> assign(:editing_target, nil) \|> put_flash(:info, "Target #{action} successfully")} {:error, form} -> {:noreply, assign(socket, :target_form, to_form(form))} end end `[Suggestion processed]` Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where leaving password fields blank during an edit would incorrectly clear the stored credentials, leading to monitoring failures and data loss.	High
	✅ ~~Fix bug losing OID input~~ Suggestion Impact: The commit addresses the same "lost edits on blur" issue but via a different implementation: it refactors the update_oid/update_template_oid handlers to accept a "field" and read the new value from params["value"] (or params["delta"] for checkboxes), and updates the UI to send phx-value-field instead of passing all fields. This prevents relying on missing/incorrect param keys and avoids losing the edited OID value. code diff: - def handle_event("update_oid", %{"index" => index_str} = params, socket) do + def handle_event("update_oid", %{"index" => index_str, "field" => field} = params, socket) do index = String.to_integer(index_str) oids = socket.assigns.target_oids - - updated_oid = - Enum.at(oids, index) - \|> Map.merge(%{ - "oid" => Map.get(params, "oid", ""), - "name" => Map.get(params, "name", ""), - "data_type" => Map.get(params, "data_type", "gauge"), - "scale" => Map.get(params, "scale", "1.0"), - "delta" => Map.get(params, "delta", "false") == "true" - }) - + current_oid = Enum.at(oids, index) + + # Get the new value for the changed field + # - For text inputs (phx-blur): fresh value is in params["value"] + # - For select (phx-change): fresh value is in params["value"] + # - For checkbox (phx-click): toggled value is in params["delta"] + new_value = + case field do + "delta" -> Map.get(params, "delta", "false") == "true" + _ -> Map.get(params, "value", "") + end + + updated_oid = Map.put(current_oid, field, new_value) updated_oids = List.replace_at(oids, index, updated_oid) {:noreply, assign(socket, :target_oids, updated_oids)} end @@ -885,20 +906,22 @@ {:noreply, assign(socket, :custom_template_oids, oids)} end - def handle_event("update_template_oid", %{"index" => index_str} = params, socket) do + def handle_event("update_template_oid", %{"index" => index_str, "field" => field} = params, socket) do index = String.to_integer(index_str) oids = socket.assigns.custom_template_oids - - updated_oid = - Enum.at(oids, index) - \|> Map.merge(%{ - "oid" => Map.get(params, "oid", ""), - "name" => Map.get(params, "name", ""), - "data_type" => Map.get(params, "data_type", "gauge"), - "scale" => Map.get(params, "scale", "1.0"), - "delta" => Map.get(params, "delta", "false") == "true" - }) - + current_oid = Enum.at(oids, index) + + # Get the new value for the changed field + # - For text inputs (phx-blur): fresh value is in params["value"] + # - For select (phx-change): fresh value is in params["value"] + # - For checkbox (phx-click): toggled value is in params["delta"] + new_value = + case field do + "delta" -> Map.get(params, "delta", "false") == "true" + _ -> Map.get(params, "value", "") + end + + updated_oid = Map.put(current_oid, field, new_value) updated_oids = List.replace_at(oids, index, updated_oid) {:noreply, assign(socket, :custom_template_oids, updated_oids)} end @@ -1939,11 +1962,7 @@ class="input input-bordered input-sm w-full font-mono text-xs" phx-blur="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-data_type={oid["data_type"]} - phx-value-scale={oid["scale"]} - phx-value-delta={to_string(oid["delta"])} + phx-value-field="oid" name={"oid_#{idx}_oid"} /> <span class="text-[10px] text-base-content/50">OID</span> @@ -1956,11 +1975,7 @@ class="input input-bordered input-sm w-full text-xs" phx-blur="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-data_type={oid["data_type"]} - phx-value-scale={oid["scale"]} - phx-value-delta={to_string(oid["delta"])} + phx-value-field="name" name={"oid_#{idx}_name"} /> <span class="text-[10px] text-base-content/50">Name</span> @@ -1970,10 +1985,7 @@ class="select select-bordered select-sm w-full text-xs" phx-change="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-scale={oid["scale"]} - phx-value-delta={to_string(oid["delta"])} + phx-value-field="data_type" name={"oid_#{idx}_data_type"} > <option value="gauge" selected={oid["data_type"] == "gauge"}>Gauge</option> @@ -1995,10 +2007,7 @@ checked={oid["delta"] == true or oid["delta"] == "true"} phx-click="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-data_type={oid["data_type"]} - phx-value-scale={oid["scale"]} + phx-value-field="delta" phx-value-delta={ to_string(!(oid["delta"] == true or oid["delta"] == "true")) } @@ -2436,10 +2445,11 @@ class="input input-bordered input-sm w-full font-mono text-xs" phx-blur="update_template_oid" phx-value-index={idx} + phx-value-field="oid" name="oid" /> </div> - + <!-- Name --> <div> <label class="label py-0"> @@ -2452,10 +2462,11 @@ class="input input-bordered input-sm w-full text-xs" phx-blur="update_template_oid" phx-value-index={idx} + phx-value-field="name" name="name" /> </div> - + <!-- Data Type --> <div> <label class="label py-0"> @@ -2465,6 +2476,7 @@ class="select select-bordered select-sm w-full text-xs" phx-change="update_template_oid" phx-value-index={idx} + phx-value-field="data_type" name="data_type" > <%= for {value, label} <- @data_types do %> @@ -2472,7 +2484,7 @@ <% end %> </select> </div> - + <!-- Scale --> <div> <label class="label py-0"> @@ -2485,10 +2497,11 @@ class="input input-bordered input-sm w-full text-xs" phx-blur="update_template_oid" phx-value-index={idx} + phx-value-field="scale" name="scale" /> </div> - + <!-- Delta checkbox --> <div class="col-span-2 flex items-center gap-2 mt-1"> <input @@ -2497,6 +2510,7 @@ class="checkbox checkbox-sm" phx-click="update_template_oid" phx-value-index={idx} + phx-value-field="delta" phx-value-delta={if oid["delta"], do: "false", else: "true"} name="delta" /> @@ -2715,22 +2729,28 @@ In the `update_oid` event handler, correctly retrieve the OID value from the `params` map using the input's unique name (`oid_#{index}_oid`) to prevent edits from being lost on blur. web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [576-592] def handle_event("update_oid", %{"index" => index_str} = params, socket) do index = String.to_integer(index_str) oids = socket.assigns.target_oids + # The `phx-blur` event on the OID input doesn't send its own value, + # so we need to get it from the params map. + oid_value = Map.get(params, "oid_#{index}_oid", Map.get(params, "oid")) + updated_oid = Enum.at(oids, index) \|> Map.merge(%{ - "oid" => Map.get(params, "oid", ""), + "oid" => oid_value, "name" => Map.get(params, "name", ""), "data_type" => Map.get(params, "data_type", "gauge"), "scale" => Map.get(params, "scale", "1.0"), "delta" => Map.get(params, "delta", "false") == "true" }) updated_oids = List.replace_at(oids, index, updated_oid) {:noreply, assign(socket, :target_oids, updated_oids)} end `[Suggestion processed]` Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a significant UI bug where user edits to an OID field were lost on blur, making the feature unusable. The fix correctly retrieves the updated value from the form parameters.	High
	✅ ~~Align filter operators~~ Suggestion Impact: The commit updated apply_field_filter to recognize "not_equals" (alongside legacy "not_eq") and added explicit handling for "contains". It also added handling for "not_contains" (grouped with "not_like"), though instead of translating it to an Ash not_contains filter it skips the negative-contains filter and leaves the query unchanged (approximate count). code diff: @@ -2715,22 +2729,28 @@ defp map_srql_field(field), do: Map.get(@srql_field_mapping, field) # Apply filter based on SRQL operator - defp apply_field_filter(query, field, "eq", value) do + # Supports both UI operators (equals, contains) and legacy operators (eq, like) + defp apply_field_filter(query, field, op, value) when op in ["eq", "equals"] do Ash.Query.filter_input(query, %{field => %{eq: value}}) end - defp apply_field_filter(query, field, "not_eq", value) do + defp apply_field_filter(query, field, op, value) when op in ["not_eq", "not_equals"] do Ash.Query.filter_input(query, %{field => %{not_eq: value}}) end + defp apply_field_filter(query, field, "contains", value) do + Ash.Query.filter_input(query, %{field => %{contains: value}}) + end + defp apply_field_filter(query, field, "like", value) do - # SRQL "like" values contain % wildcards, strip them for Ash contains + # Legacy SRQL "like" values contain % wildcards, strip them for Ash contains stripped = value \|> String.trim_leading("%") \|> String.trim_trailing("%") Ash.Query.filter_input(query, %{field => %{contains: stripped}}) end - defp apply_field_filter(query, _field, "not_like", _value) do - # Skip not_like - count will be an approximation + defp apply_field_filter(query, _field, op, _value) when op in ["not_like", "not_contains"] do + # Skip negative contains - count will be an approximation + # Ash doesn't have a direct not_contains filter query end In `apply_field_filter`, align the function clauses with the SRQL parser's output by handling `contains`, `not_contains`, and `not_equals` operators to ensure correct filter translation. web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [2718-2730] defp apply_field_filter(query, field, "eq", value) do Ash.Query.filter_input(query, %{field => %{eq: value}}) end -defp apply_field_filter(query, field, "not_eq", value) do +defp apply_field_filter(query, field, "not_equals", value) do Ash.Query.filter_input(query, %{field => %{not_eq: value}}) end -defp apply_field_filter(query, field, "like", value) do - # SRQL "like" values contain % wildcards, strip them for Ash contains - stripped = value \|> String.trim_leading("%") \|> String.trim_trailing("%") - Ash.Query.filter_input(query, %{field => %{contains: stripped}}) +defp apply_field_filter(query, field, "contains", value) do + Ash.Query.filter_input(query, %{field => %{contains: value}}) end +defp apply_field_filter(query, field, "not_contains", value) do + Ash.Query.filter_input(query, %{field => %{not_contains: value}}) +end + +defp apply_field_filter(query, field, _op, value) do + Ash.Query.filter_input(query, %{field => %{eq: value}}) +end + `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: This suggestion fixes a bug where SRQL filter operators were incorrectly mapped, causing the device count preview to be inaccurate. The fix aligns the filter logic with the parser's output.	Medium
	Match template id as string In the `add_template_oids` event handler, convert the template's ID to a string before comparing it with the `template_id` from the event parameters to fix a type mismatch bug. web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [617] `-case Enum.find(templates, &(&1.id == template_id)) do +case Enum.find(templates, fn t -> to_string(t.id) == template_id end) do` Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion fixes a type mismatch bug where a string `template_id` from the client would never match the template's ID, causing the "Add Template OIDs" feature to fail silently.	Medium
	✅ ~~Properly handle SNMPv3 authentication parameters~~ Suggestion Impact: The commit removes the "temporary workaround" comments and now populates a dedicated SNMPv3 auth struct (target.V3Auth) with username, security level, auth/priv protocols and passwords, along with adding the required proto-to-SNMP conversion helpers. It still clears target.Community for v3. While it doesn't match the exact suggested fields (e.g., target.V3Params/target.Version), it implements the intended proper handling of SNMPv3 auth parameters. code diff: @@ -664,9 +664,14 @@ // Handle SNMPv3 auth if t.V3Auth != nil { target.Community = "" // Clear community for v3 - // V3 auth is handled separately in the collector - // For now we store username as community (temporary workaround) - // TODO: Add proper v3 auth support to Target struct + target.V3Auth = &snmp.V3Auth{ + Username: t.V3Auth.Username, + SecurityLevel: protoToSNMPSecurityLevel(t.V3Auth.SecurityLevel), + AuthProtocol: protoToSNMPAuthProtocol(t.V3Auth.AuthProtocol), + AuthPassword: t.V3Auth.AuthPassword, + PrivProtocol: protoToSNMPPrivProtocol(t.V3Auth.PrivProtocol), + PrivPassword: t.V3Auth.PrivPassword, + } } for _, oid := range t.Oids { @@ -722,3 +727,58 @@ } return snmp.TypeGauge } + +// protoToSNMPSecurityLevel converts proto SNMPSecurityLevel to snmp.SecurityLevel. +func protoToSNMPSecurityLevel(sl proto.SNMPSecurityLevel) snmp.SecurityLevel { + switch sl { + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_NO_AUTH_NO_PRIV: + return snmp.SecurityLevelNoAuthNoPriv + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_AUTH_NO_PRIV: + return snmp.SecurityLevelAuthNoPriv + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_AUTH_PRIV: + return snmp.SecurityLevelAuthPriv + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_UNSPECIFIED: + return snmp.SecurityLevelNoAuthNoPriv + } + return snmp.SecurityLevelNoAuthNoPriv +} + +// protoToSNMPAuthProtocol converts proto SNMPAuthProtocol to snmp.AuthProtocol. +func protoToSNMPAuthProtocol(ap proto.SNMPAuthProtocol) snmp.AuthProtocol { + switch ap { + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_MD5: + return snmp.AuthProtocolMD5 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA: + return snmp.AuthProtocolSHA + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA224: + return snmp.AuthProtocolSHA224 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA256: + return snmp.AuthProtocolSHA256 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA384: + return snmp.AuthProtocolSHA384 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA512: + return snmp.AuthProtocolSHA512 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_UNSPECIFIED: + return snmp.AuthProtocolMD5 + } + return snmp.AuthProtocolMD5 +} + +// protoToSNMPPrivProtocol converts proto SNMPPrivProtocol to snmp.PrivProtocol. +func protoToSNMPPrivProtocol(pp proto.SNMPPrivProtocol) snmp.PrivProtocol { + switch pp { + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_DES: + return snmp.PrivProtocolDES + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES: + return snmp.PrivProtocolAES + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES192, + proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES192C: + return snmp.PrivProtocolAES192 + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES256, + proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES256C: + return snmp.PrivProtocolAES256 + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_UNSPECIFIED: + return snmp.PrivProtocolDES + } + return snmp.PrivProtocolDES +} Implement proper SNMPv3 authentication parameter handling instead of using the current workaround which stores a username in the community string field. pkg/agent/snmp_service.go [664-670] // Handle SNMPv3 auth if t.V3Auth != nil { - target.Community = "" // Clear community for v3 - // V3 auth is handled separately in the collector - // For now we store username as community (temporary workaround) - // TODO: Add proper v3 auth support to Target struct + target.Version = snmp.Version3 + target.V3 = &snmp.V3Params{ + Username: t.V3Auth.Username, + SecurityLevel: protoToSNMPSecurityLevel(t.V3Auth.SecurityLevel), + AuthProtocol: protoToSNMPAuthProtocol(t.V3Auth.AuthProtocol), + AuthPassword: t.V3Auth.AuthPassword, + PrivProtocol: protoToSNMPPrivProtocol(t.V3Auth.PrivProtocol), + PrivPassword: t.V3Auth.PrivPassword, + } + target.Community = "" // Ensure community is not used for v3 } `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant issue where SNMPv3 authentication is handled via a non-functional workaround, which would cause authentication to fail.	Medium
	✅ ~~Safely parse port input~~ Suggestion Impact: The port parsing logic was updated to use Integer.parse/1 for binary inputs and fall back to 161 on parse errors, preventing crashes from non-numeric port values. code diff: `port = case port do p when is_integer(p) -> p - p when is_binary(p) -> String.to_integer(p) + + p when is_binary(p) -> + case Integer.parse(p) do + {int, _} -> int + :error -> 161 + end + _ -> 161 end` In the `test_connection` event handler, replace `String.to_integer/1` with the safer `Integer.parse/1` to handle non-numeric port inputs gracefully and prevent crashes. web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [529-534] `port = case port do p when is_integer(p) -> p - p when is_binary(p) -> String.to_integer(p) + p when is_binary(p) -> + case Integer.parse(p) do + {int, _} -> int + :error -> 161 + end _ -> 161 end` `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that `String.to_integer/1` can crash with invalid input. Using `Integer.parse/1` makes the port parsing more robust and prevents potential process crashes.	Medium
General	✅ ~~Skip disabled profiles~~ Suggestion Impact: Updated the conditional to require both a resolved profile and profile.enabled before compiling; otherwise returns disabled_config, including for disabled profiles. Also updated the comment accordingly. code diff: `- if profile do + if profile && profile.enabled do config = compile_profile(profile, tenant_schema, actor) {:ok, config} else - # Return disabled config if no profile found + # Return disabled config if no profile found or profile is disabled {:ok, disabled_config()}` Check if a profile is enabled before compiling it; if it is disabled, return the disabled configuration to avoid unnecessary processing. elixir/serviceradar_core/lib/serviceradar/agent_config/compilers/snmp_compiler.ex [81-87] `-if profile do +if profile && profile.enabled do config = compile_profile(profile, tenant_schema, actor) {:ok, config} else {:ok, disabled_config()} end` `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: This is a good optimization that avoids unnecessarily compiling a disabled profile, making the logic more efficient and consistent with the case where no profile is found.	Medium
	Prevent connection test race condition In the `test_connection` event handler, add a check for the `test_connection_loading` flag to prevent multiple concurrent connection tests and avoid potential race conditions. web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [521-552] def handle_event("test_connection", _params, socket) do - form = socket.assigns.target_form + if socket.assigns.test_connection_loading do + {:noreply, socket} + else + form = socket.assigns.target_form - # Get form values - handle both source formats - host = get_form_value(form, :host, "") - port = get_form_value(form, :port, 161) + # Get form values - handle both source formats + host = get_form_value(form, :host, "") + port = get_form_value(form, :port, 161) - # Convert port to integer if needed - port = - case port do - p when is_integer(p) -> p - p when is_binary(p) -> String.to_integer(p) - _ -> 161 + # Convert port to integer if needed + port = + case port do + p when is_integer(p) -> p + p when is_binary(p) -> String.to_integer(p) + _ -> 161 + end + + # Validate we have a host + if host == "" or host == nil do + {:noreply, + assign(socket, :test_connection_result, %{ + success: false, + message: "Please enter a host address first" + })} + else + # Set loading state + socket = assign(socket, :test_connection_loading, true) + + # Send to self to do async work + send(self(), {:test_snmp_connection, host, port}) + + {:noreply, socket} end - - # Validate we have a host - if host == "" or host == nil do - {:noreply, - assign(socket, :test_connection_result, %{ - success: false, - message: "Please enter a host address first" - })} - else - # Set loading state - socket = assign(socket, :test_connection_loading, true) - - # Send to self to do async work - send(self(), {:test_snmp_connection, host, port}) - - {:noreply, socket} end end Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential race condition from multiple clicks on the "Test Connection" button and provides a simple, effective fix by checking the loading state, improving UI stability.	Low
	✅ ~~Use bulk update for efficiency~~ Suggestion Impact: The commit removed the Enum.each loop that built and updated individual changesets and replaced it with a single Ash.bulk_update call to perform the unset_default action for all profiles at once. code diff: `case Ash.read(query, actor: actor) do {:ok, profiles} -> - # Unset each default profile using the dedicated action - Enum.each(profiles, fn profile -> - profile - \|> Ash.Changeset.for_update(:unset_default, %{}, actor: actor, tenant: tenant) - \|> Ash.update(actor: actor) - end) + # Unset all default profiles in a single bulk operation + Ash.bulk_update(profiles, :unset_default, %{}, actor: actor, tenant: tenant)` Use `Ash.bulk_update/4` to unset previous default profiles in a single, more efficient database operation instead of iterating and updating them individually. elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/set_as_default.ex [37-52] case Ash.read(query, actor: actor) do {:ok, profiles} -> - # Unset each default profile using the dedicated action - Enum.each(profiles, fn profile -> - profile - \|> Ash.Changeset.for_update(:unset_default, %{}, actor: actor, tenant: tenant) - \|> Ash.update(actor: actor) - end) + # Unset each default profile using the dedicated action in a single bulk operation + Ash.bulk_update(profiles, :unset_default, %{}, actor: actor, tenant: tenant) {:error, error} -> # Log but don't fail - the system should continue and we'll have multiple defaults # which is still better than crashing require Logger Logger.warning("Failed to query existing default SNMP profiles: #{inspect(error)}") :ok end `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: The suggestion proposes using `Ash.bulk_update/4` for better performance and atomicity, which is a valid and good practice for updating multiple records.	Low
	✅ ~~Trim and concatenate SRQL query~~ Suggestion Impact: Updated combined_query construction to use String.trim(target_query) <> " uid:" <> device_uid, preventing malformed SRQL queries due to extra/missing whitespace. code diff: `- combined_query = "#{target_query} uid:#{device_uid}" + combined_query = String.trim(target_query) <> " uid:" <> device_uid` In `matches_device?/4`, trim whitespace from `target_query` and ensure a space is present before concatenating the `uid` filter to prevent malformed SRQL queries. elixir/serviceradar_core/lib/serviceradar/snmp_profiles/srql_target_resolver.ex [134] `-combined_query = "#{target_query} uid:#{device_uid}" +combined_query = String.trim(target_query) <> " uid:" <> device_uid` `[Suggestion processed]` Suggestion importance[1-10]: 5 __ Why: This change improves the robustness of SRQL query construction by handling potential leading/trailing whitespace in `target_query`, preventing malformed queries.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2298#issuecomment-3750712054 Original created: 2026-01-14T17:29:03Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=6>Possible issue</td> <td> <details><summary>✅ <s>Prevent accidental deletion of credentials</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit adds logic in handle_event("save_target") to reject empty values for community/auth_password/priv_password when editing a target, preventing accidental clearing of stored encrypted credentials. (Other unrelated changes were also included.) code diff: ```diff @@ -460,6 +460,19 @@ scope = socket.assigns.current_scope profile_id = socket.assigns.selected_profile.id + # When editing, remove blank password/community fields from params + # to avoid accidentally clearing existing encrypted credentials. + params = + if socket.assigns.editing_target do + sensitive_fields = ["community", "auth_password", "priv_password"] + + Map.reject(params, fn {key, value} -> + key in sensitive_fields and value == "" + end) + else + params + end + target_form = if socket.assigns.editing_target do Form.for_update(socket.assigns.editing_target, :update, @@ -529,7 +542,13 @@ ``` </details> ___ **In the <code>save_target</code> event handler, when editing a target, remove blank password and community string fields from the submitted parameters to avoid unintentionally clearing existing credentials.** [web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [459-495]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-1493b4100054d62c0d3fdc343e3fb7ce5f21735f752fad7d0fc978ae1ba114e4R459-R495) ```diff def handle_event("save_target", %{"form" => params}, socket) do scope = socket.assigns.current_scope profile_id = socket.assigns.selected_profile.id + + # When editing, remove blank password/community fields from params + # to avoid accidentally clearing them. + params = + if socket.assigns.editing_target do + params + |> Map.delete_if(fn key, value -> + key in ["community", "auth_password", "priv_password"] and value == "" + end) + else + params + end target_form = if socket.assigns.editing_target do Form.for_update(socket.assigns.editing_target, :update, domain: ServiceRadar.SNMPProfiles, scope: scope ) else Form.for_create(SNMPTarget, :create, domain: ServiceRadar.SNMPProfiles, scope: scope, params: %{snmp_profile_id: profile_id} ) end target_form = Form.validate(target_form, params) case Form.submit(target_form, params: params) do {:ok, _target} -> action = if socket.assigns.editing_target, do: "updated", else: "created" targets = load_profile_targets(scope, profile_id) {:noreply, socket |> assign(:targets, targets) |> assign(:show_target_modal, false) |> assign(:target_form, nil) |> assign(:editing_target, nil) |> put_flash(:info, "Target #{action} successfully")} {:error, form} -> {:noreply, assign(socket, :target_form, to_form(form))} end end ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion fixes a critical bug where leaving password fields blank during an edit would incorrectly clear the stored credentials, leading to monitoring failures and data loss. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>✅ <s>Fix bug losing OID input</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit addresses the same "lost edits on blur" issue but via a different implementation: it refactors the update_oid/update_template_oid handlers to accept a "field" and read the new value from params["value"] (or params["delta"] for checkboxes), and updates the UI to send phx-value-field instead of passing all fields. This prevents relying on missing/incorrect param keys and avoids losing the edited OID value. code diff: ```diff - def handle_event("update_oid", %{"index" => index_str} = params, socket) do + def handle_event("update_oid", %{"index" => index_str, "field" => field} = params, socket) do index = String.to_integer(index_str) oids = socket.assigns.target_oids - - updated_oid = - Enum.at(oids, index) - |> Map.merge(%{ - "oid" => Map.get(params, "oid", ""), - "name" => Map.get(params, "name", ""), - "data_type" => Map.get(params, "data_type", "gauge"), - "scale" => Map.get(params, "scale", "1.0"), - "delta" => Map.get(params, "delta", "false") == "true" - }) - + current_oid = Enum.at(oids, index) + + # Get the new value for the changed field + # - For text inputs (phx-blur): fresh value is in params["value"] + # - For select (phx-change): fresh value is in params["value"] + # - For checkbox (phx-click): toggled value is in params["delta"] + new_value = + case field do + "delta" -> Map.get(params, "delta", "false") == "true" + _ -> Map.get(params, "value", "") + end + + updated_oid = Map.put(current_oid, field, new_value) updated_oids = List.replace_at(oids, index, updated_oid) {:noreply, assign(socket, :target_oids, updated_oids)} end @@ -885,20 +906,22 @@ {:noreply, assign(socket, :custom_template_oids, oids)} end - def handle_event("update_template_oid", %{"index" => index_str} = params, socket) do + def handle_event("update_template_oid", %{"index" => index_str, "field" => field} = params, socket) do index = String.to_integer(index_str) oids = socket.assigns.custom_template_oids - - updated_oid = - Enum.at(oids, index) - |> Map.merge(%{ - "oid" => Map.get(params, "oid", ""), - "name" => Map.get(params, "name", ""), - "data_type" => Map.get(params, "data_type", "gauge"), - "scale" => Map.get(params, "scale", "1.0"), - "delta" => Map.get(params, "delta", "false") == "true" - }) - + current_oid = Enum.at(oids, index) + + # Get the new value for the changed field + # - For text inputs (phx-blur): fresh value is in params["value"] + # - For select (phx-change): fresh value is in params["value"] + # - For checkbox (phx-click): toggled value is in params["delta"] + new_value = + case field do + "delta" -> Map.get(params, "delta", "false") == "true" + _ -> Map.get(params, "value", "") + end + + updated_oid = Map.put(current_oid, field, new_value) updated_oids = List.replace_at(oids, index, updated_oid) {:noreply, assign(socket, :custom_template_oids, updated_oids)} end @@ -1939,11 +1962,7 @@ class="input input-bordered input-sm w-full font-mono text-xs" phx-blur="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-data_type={oid["data_type"]} - phx-value-scale={oid["scale"]} - phx-value-delta={to_string(oid["delta"])} + phx-value-field="oid" name={"oid_#{idx}_oid"} /> OID @@ -1956,11 +1975,7 @@ class="input input-bordered input-sm w-full text-xs" phx-blur="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-data_type={oid["data_type"]} - phx-value-scale={oid["scale"]} - phx-value-delta={to_string(oid["delta"])} + phx-value-field="name" name={"oid_#{idx}_name"} /> Name @@ -1970,10 +1985,7 @@ class="select select-bordered select-sm w-full text-xs" phx-change="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-scale={oid["scale"]} - phx-value-delta={to_string(oid["delta"])} + phx-value-field="data_type" name={"oid_#{idx}_data_type"} > <option value="gauge" selected={oid["data_type"] == "gauge"}>Gauge</option> @@ -1995,10 +2007,7 @@ checked={oid["delta"] == true or oid["delta"] == "true"} phx-click="update_oid" phx-value-index={idx} - phx-value-oid={oid["oid"]} - phx-value-name={oid["name"]} - phx-value-data_type={oid["data_type"]} - phx-value-scale={oid["scale"]} + phx-value-field="delta" phx-value-delta={ to_string(!(oid["delta"] == true or oid["delta"] == "true")) } @@ -2436,10 +2445,11 @@ class="input input-bordered input-sm w-full font-mono text-xs" phx-blur="update_template_oid" phx-value-index={idx} + phx-value-field="oid" name="oid" /> </div> - +  <div> <label class="label py-0"> @@ -2452,10 +2462,11 @@ class="input input-bordered input-sm w-full text-xs" phx-blur="update_template_oid" phx-value-index={idx} + phx-value-field="name" name="name" /> </div> - +  <div> <label class="label py-0"> @@ -2465,6 +2476,7 @@ class="select select-bordered select-sm w-full text-xs" phx-change="update_template_oid" phx-value-index={idx} + phx-value-field="data_type" name="data_type" > <%= for {value, label} <- @data_types do %> @@ -2472,7 +2484,7 @@ <% end %> </select> </div> - +  <div> <label class="label py-0"> @@ -2485,10 +2497,11 @@ class="input input-bordered input-sm w-full text-xs" phx-blur="update_template_oid" phx-value-index={idx} + phx-value-field="scale" name="scale" /> </div> - +  <div class="col-span-2 flex items-center gap-2 mt-1"> <input @@ -2497,6 +2510,7 @@ class="checkbox checkbox-sm" phx-click="update_template_oid" phx-value-index={idx} + phx-value-field="delta" phx-value-delta={if oid["delta"], do: "false", else: "true"} name="delta" /> @@ -2715,22 +2729,28 @@ ``` </details> ___ **In the <code>update_oid</code> event handler, correctly retrieve the OID value from the <code>params</code> map using the input's unique name (<code>oid_#{index}_oid</code>) to prevent edits from being lost on blur.** [web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [576-592]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-1493b4100054d62c0d3fdc343e3fb7ce5f21735f752fad7d0fc978ae1ba114e4R576-R592) ```diff def handle_event("update_oid", %{"index" => index_str} = params, socket) do index = String.to_integer(index_str) oids = socket.assigns.target_oids + # The `phx-blur` event on the OID input doesn't send its own value, + # so we need to get it from the params map. + oid_value = Map.get(params, "oid_#{index}_oid", Map.get(params, "oid")) + updated_oid = Enum.at(oids, index) |> Map.merge(%{ - "oid" => Map.get(params, "oid", ""), + "oid" => oid_value, "name" => Map.get(params, "name", ""), "data_type" => Map.get(params, "data_type", "gauge"), "scale" => Map.get(params, "scale", "1.0"), "delta" => Map.get(params, "delta", "false") == "true" }) updated_oids = List.replace_at(oids, index, updated_oid) {:noreply, assign(socket, :target_oids, updated_oids)} end ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion fixes a significant UI bug where user edits to an OID field were lost on blur, making the feature unusable. The fix correctly retrieves the updated value from the form parameters. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>✅ <s>Align filter operators</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit updated apply_field_filter to recognize "not_equals" (alongside legacy "not_eq") and added explicit handling for "contains". It also added handling for "not_contains" (grouped with "not_like"), though instead of translating it to an Ash not_contains filter it skips the negative-contains filter and leaves the query unchanged (approximate count). code diff: ```diff @@ -2715,22 +2729,28 @@ defp map_srql_field(field), do: Map.get(@srql_field_mapping, field) # Apply filter based on SRQL operator - defp apply_field_filter(query, field, "eq", value) do + # Supports both UI operators (equals, contains) and legacy operators (eq, like) + defp apply_field_filter(query, field, op, value) when op in ["eq", "equals"] do Ash.Query.filter_input(query, %{field => %{eq: value}}) end - defp apply_field_filter(query, field, "not_eq", value) do + defp apply_field_filter(query, field, op, value) when op in ["not_eq", "not_equals"] do Ash.Query.filter_input(query, %{field => %{not_eq: value}}) end + defp apply_field_filter(query, field, "contains", value) do + Ash.Query.filter_input(query, %{field => %{contains: value}}) + end + defp apply_field_filter(query, field, "like", value) do - # SRQL "like" values contain % wildcards, strip them for Ash contains + # Legacy SRQL "like" values contain % wildcards, strip them for Ash contains stripped = value |> String.trim_leading("%") |> String.trim_trailing("%") Ash.Query.filter_input(query, %{field => %{contains: stripped}}) end - defp apply_field_filter(query, _field, "not_like", _value) do - # Skip not_like - count will be an approximation + defp apply_field_filter(query, _field, op, _value) when op in ["not_like", "not_contains"] do + # Skip negative contains - count will be an approximation + # Ash doesn't have a direct not_contains filter query end ``` </details> ___ **In <code>apply_field_filter</code>, align the function clauses with the SRQL parser's output by handling <code>contains</code>, <code>not_contains</code>, and <code>not_equals</code> operators to ensure correct filter translation.** [web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [2718-2730]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-1493b4100054d62c0d3fdc343e3fb7ce5f21735f752fad7d0fc978ae1ba114e4R2718-R2730) ```diff defp apply_field_filter(query, field, "eq", value) do Ash.Query.filter_input(query, %{field => %{eq: value}}) end -defp apply_field_filter(query, field, "not_eq", value) do +defp apply_field_filter(query, field, "not_equals", value) do Ash.Query.filter_input(query, %{field => %{not_eq: value}}) end -defp apply_field_filter(query, field, "like", value) do - # SRQL "like" values contain % wildcards, strip them for Ash contains - stripped = value |> String.trim_leading("%") |> String.trim_trailing("%") - Ash.Query.filter_input(query, %{field => %{contains: stripped}}) +defp apply_field_filter(query, field, "contains", value) do + Ash.Query.filter_input(query, %{field => %{contains: value}}) end +defp apply_field_filter(query, field, "not_contains", value) do + Ash.Query.filter_input(query, %{field => %{not_contains: value}}) +end + +defp apply_field_filter(query, field, _op, value) do + Ash.Query.filter_input(query, %{field => %{eq: value}}) +end + ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This suggestion fixes a bug where SRQL filter operators were incorrectly mapped, causing the device count preview to be inaccurate. The fix aligns the filter logic with the parser's output. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Match template id as string</summary> ___ **In the <code>add_template_oids</code> event handler, convert the template's ID to a string before comparing it with the <code>template_id</code> from the event parameters to fix a type mismatch bug.** [web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [617]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-1493b4100054d62c0d3fdc343e3fb7ce5f21735f752fad7d0fc978ae1ba114e4R617-R617) ```diff -case Enum.find(templates, &(&1.id == template_id)) do +case Enum.find(templates, fn t -> to_string(t.id) == template_id end) do ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This suggestion fixes a type mismatch bug where a string `template_id` from the client would never match the template's ID, causing the "Add Template OIDs" feature to fail silently. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>✅ <s>Properly handle SNMPv3 authentication parameters</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit removes the "temporary workaround" comments and now populates a dedicated SNMPv3 auth struct (target.V3Auth) with username, security level, auth/priv protocols and passwords, along with adding the required proto-to-SNMP conversion helpers. It still clears target.Community for v3. While it doesn't match the exact suggested fields (e.g., target.V3Params/target.Version), it implements the intended proper handling of SNMPv3 auth parameters. code diff: ```diff @@ -664,9 +664,14 @@ // Handle SNMPv3 auth if t.V3Auth != nil { target.Community = "" // Clear community for v3 - // V3 auth is handled separately in the collector - // For now we store username as community (temporary workaround) - // TODO: Add proper v3 auth support to Target struct + target.V3Auth = &snmp.V3Auth{ + Username: t.V3Auth.Username, + SecurityLevel: protoToSNMPSecurityLevel(t.V3Auth.SecurityLevel), + AuthProtocol: protoToSNMPAuthProtocol(t.V3Auth.AuthProtocol), + AuthPassword: t.V3Auth.AuthPassword, + PrivProtocol: protoToSNMPPrivProtocol(t.V3Auth.PrivProtocol), + PrivPassword: t.V3Auth.PrivPassword, + } } for _, oid := range t.Oids { @@ -722,3 +727,58 @@ } return snmp.TypeGauge } + +// protoToSNMPSecurityLevel converts proto SNMPSecurityLevel to snmp.SecurityLevel. +func protoToSNMPSecurityLevel(sl proto.SNMPSecurityLevel) snmp.SecurityLevel { + switch sl { + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_NO_AUTH_NO_PRIV: + return snmp.SecurityLevelNoAuthNoPriv + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_AUTH_NO_PRIV: + return snmp.SecurityLevelAuthNoPriv + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_AUTH_PRIV: + return snmp.SecurityLevelAuthPriv + case proto.SNMPSecurityLevel_SNMP_SECURITY_LEVEL_UNSPECIFIED: + return snmp.SecurityLevelNoAuthNoPriv + } + return snmp.SecurityLevelNoAuthNoPriv +} + +// protoToSNMPAuthProtocol converts proto SNMPAuthProtocol to snmp.AuthProtocol. +func protoToSNMPAuthProtocol(ap proto.SNMPAuthProtocol) snmp.AuthProtocol { + switch ap { + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_MD5: + return snmp.AuthProtocolMD5 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA: + return snmp.AuthProtocolSHA + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA224: + return snmp.AuthProtocolSHA224 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA256: + return snmp.AuthProtocolSHA256 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA384: + return snmp.AuthProtocolSHA384 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_SHA512: + return snmp.AuthProtocolSHA512 + case proto.SNMPAuthProtocol_SNMP_AUTH_PROTOCOL_UNSPECIFIED: + return snmp.AuthProtocolMD5 + } + return snmp.AuthProtocolMD5 +} + +// protoToSNMPPrivProtocol converts proto SNMPPrivProtocol to snmp.PrivProtocol. +func protoToSNMPPrivProtocol(pp proto.SNMPPrivProtocol) snmp.PrivProtocol { + switch pp { + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_DES: + return snmp.PrivProtocolDES + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES: + return snmp.PrivProtocolAES + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES192, + proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES192C: + return snmp.PrivProtocolAES192 + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES256, + proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_AES256C: + return snmp.PrivProtocolAES256 + case proto.SNMPPrivProtocol_SNMP_PRIV_PROTOCOL_UNSPECIFIED: + return snmp.PrivProtocolDES + } + return snmp.PrivProtocolDES +} ``` </details> ___ **Implement proper SNMPv3 authentication parameter handling instead of using the current workaround which stores a username in the community string field.** [pkg/agent/snmp_service.go [664-670]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-703d839c384ade98beb93a20adb32586e2035374ce27ca4861f7cdee656e1699R664-R670) ```diff // Handle SNMPv3 auth if t.V3Auth != nil { - target.Community = "" // Clear community for v3 - // V3 auth is handled separately in the collector - // For now we store username as community (temporary workaround) - // TODO: Add proper v3 auth support to Target struct + target.Version = snmp.Version3 + target.V3 = &snmp.V3Params{ + Username: t.V3Auth.Username, + SecurityLevel: protoToSNMPSecurityLevel(t.V3Auth.SecurityLevel), + AuthProtocol: protoToSNMPAuthProtocol(t.V3Auth.AuthProtocol), + AuthPassword: t.V3Auth.AuthPassword, + PrivProtocol: protoToSNMPPrivProtocol(t.V3Auth.PrivProtocol), + PrivPassword: t.V3Auth.PrivPassword, + } + target.Community = "" // Ensure community is not used for v3 } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a significant issue where SNMPv3 authentication is handled via a non-functional workaround, which would cause authentication to fail. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>✅ <s>Safely parse port input</s></summary> ___ <details><summary>Suggestion Impact:</summary>The port parsing logic was updated to use Integer.parse/1 for binary inputs and fall back to 161 on parse errors, preventing crashes from non-numeric port values. code diff: ```diff port = case port do p when is_integer(p) -> p - p when is_binary(p) -> String.to_integer(p) + + p when is_binary(p) -> + case Integer.parse(p) do + {int, _} -> int + :error -> 161 + end + _ -> 161 end ``` </details> ___ **In the <code>test_connection</code> event handler, replace <code>String.to_integer/1</code> with the safer <code>Integer.parse/1</code> to handle non-numeric port inputs gracefully and prevent crashes.** [web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [529-534]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-1493b4100054d62c0d3fdc343e3fb7ce5f21735f752fad7d0fc978ae1ba114e4R529-R534) ```diff port = case port do p when is_integer(p) -> p - p when is_binary(p) -> String.to_integer(p) + p when is_binary(p) -> + case Integer.parse(p) do + {int, _} -> int + :error -> 161 + end _ -> 161 end ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that `String.to_integer/1` can crash with invalid input. Using `Integer.parse/1` makes the port parsing more robust and prevents potential process crashes. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=4>General</td> <td> <details><summary>✅ <s>Skip disabled profiles</s></summary> ___ <details><summary>Suggestion Impact:</summary>Updated the conditional to require both a resolved profile and profile.enabled before compiling; otherwise returns disabled_config, including for disabled profiles. Also updated the comment accordingly. code diff: ```diff - if profile do + if profile && profile.enabled do config = compile_profile(profile, tenant_schema, actor) {:ok, config} else - # Return disabled config if no profile found + # Return disabled config if no profile found or profile is disabled {:ok, disabled_config()} ``` </details> ___ **Check if a profile is enabled before compiling it; if it is disabled, return the disabled configuration to avoid unnecessary processing.** [elixir/serviceradar_core/lib/serviceradar/agent_config/compilers/snmp_compiler.ex [81-87]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-3f9551d39c5e77c379f701d5dee9c0a3d95e2a7861ba7515c402e10b585c61e0R81-R87) ```diff -if profile do +if profile && profile.enabled do config = compile_profile(profile, tenant_schema, actor) {:ok, config} else {:ok, disabled_config()} end ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: This is a good optimization that avoids unnecessarily compiling a disabled profile, making the logic more efficient and consistent with the case where no profile is found. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Prevent connection test race condition</summary> ___ **In the <code>test_connection</code> event handler, add a check for the <code>test_connection_loading</code> flag to prevent multiple concurrent connection tests and avoid potential race conditions.** [web-ng/lib/serviceradar_web_ng_web/live/settings/snmp_profiles_live/index.ex [521-552]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-1493b4100054d62c0d3fdc343e3fb7ce5f21735f752fad7d0fc978ae1ba114e4R521-R552) ```diff def handle_event("test_connection", _params, socket) do - form = socket.assigns.target_form + if socket.assigns.test_connection_loading do + {:noreply, socket} + else + form = socket.assigns.target_form - # Get form values - handle both source formats - host = get_form_value(form, :host, "") - port = get_form_value(form, :port, 161) + # Get form values - handle both source formats + host = get_form_value(form, :host, "") + port = get_form_value(form, :port, 161) - # Convert port to integer if needed - port = - case port do - p when is_integer(p) -> p - p when is_binary(p) -> String.to_integer(p) - _ -> 161 + # Convert port to integer if needed + port = + case port do + p when is_integer(p) -> p + p when is_binary(p) -> String.to_integer(p) + _ -> 161 + end + + # Validate we have a host + if host == "" or host == nil do + {:noreply, + assign(socket, :test_connection_result, %{ + success: false, + message: "Please enter a host address first" + })} + else + # Set loading state + socket = assign(socket, :test_connection_loading, true) + + # Send to self to do async work + send(self(), {:test_snmp_connection, host, port}) + + {:noreply, socket} end - - # Validate we have a host - if host == "" or host == nil do - {:noreply, - assign(socket, :test_connection_result, %{ - success: false, - message: "Please enter a host address first" - })} - else - # Set loading state - socket = assign(socket, :test_connection_loading, true) - - # Send to self to do async work - send(self(), {:test_snmp_connection, host, port}) - - {:noreply, socket} end end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a potential race condition from multiple clicks on the "Test Connection" button and provides a simple, effective fix by checking the loading state, improving UI stability. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>✅ <s>Use bulk update for efficiency</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit removed the Enum.each loop that built and updated individual changesets and replaced it with a single Ash.bulk_update call to perform the unset_default action for all profiles at once. code diff: ```diff case Ash.read(query, actor: actor) do {:ok, profiles} -> - # Unset each default profile using the dedicated action - Enum.each(profiles, fn profile -> - profile - |> Ash.Changeset.for_update(:unset_default, %{}, actor: actor, tenant: tenant) - |> Ash.update(actor: actor) - end) + # Unset all default profiles in a single bulk operation + Ash.bulk_update(profiles, :unset_default, %{}, actor: actor, tenant: tenant) ``` </details> ___ **Use <code>Ash.bulk_update/4</code> to unset previous default profiles in a single, more efficient database operation instead of iterating and updating them individually.** [elixir/serviceradar_core/lib/serviceradar/snmp_profiles/changes/set_as_default.ex [37-52]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-24900bdf51eca0e0319e2891a5ed38b0db74f9ff7dfcc316d8d0929d8306d95cR37-R52) ```diff case Ash.read(query, actor: actor) do {:ok, profiles} -> - # Unset each default profile using the dedicated action - Enum.each(profiles, fn profile -> - profile - |> Ash.Changeset.for_update(:unset_default, %{}, actor: actor, tenant: tenant) - |> Ash.update(actor: actor) - end) + # Unset each default profile using the dedicated action in a single bulk operation + Ash.bulk_update(profiles, :unset_default, %{}, actor: actor, tenant: tenant) {:error, error} -> # Log but don't fail - the system should continue and we'll have multiple defaults # which is still better than crashing require Logger Logger.warning("Failed to query existing default SNMP profiles: #{inspect(error)}") :ok end ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion proposes using `Ash.bulk_update/4` for better performance and atomicity, which is a valid and good practice for updating multiple records. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>✅ <s>Trim and concatenate SRQL query</s></summary> ___ <details><summary>Suggestion Impact:</summary>Updated combined_query construction to use String.trim(target_query) <> " uid:" <> device_uid, preventing malformed SRQL queries due to extra/missing whitespace. code diff: ```diff - combined_query = "#{target_query} uid:#{device_uid}" + combined_query = String.trim(target_query) <> " uid:" <> device_uid ``` </details> ___ **In <code>matches_device?/4</code>, trim whitespace from <code>target_query</code> and ensure a space is present before concatenating the <code>uid</code> filter to prevent malformed SRQL queries.** [elixir/serviceradar_core/lib/serviceradar/snmp_profiles/srql_target_resolver.ex [134]](https://github.com/carverauto/serviceradar/pull/2298/files#diff-2007e921214ed2cefc6170d60d4c2af9402cc499be1cd15737ec20344b2308acR134-R134) ```diff -combined_query = "#{target_query} uid:#{device_uid}" +combined_query = String.trim(target_query) <> " uid:" <> device_uid ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: This change improves the robustness of SRQL query construction by handling potential leading/trailing whitespace in `target_query`, preventing malformed queries. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>