carverauto/serviceradar

Fork 0

adding tests for sweep compiler #2702

Merged

mfreeman451 merged 2 commits from refs/pull/2702/head into staging

2026-01-18 20:29:27 +00:00

mfreeman451 commented

2026-01-18 20:19:18 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2357
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2357
Original created: 2026-01-18T20:19:18Z
Original updated: 2026-01-18T20:29:28Z
Original head: carverauto/serviceradar:bug/agent-not-getting-sweep-config
Original base: staging
Original merged: 2026-01-18T20:29:27Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

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

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

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

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

PR Type

Bug fix, Tests, Enhancement

Description

Fix sweep group targeting rules not persisting through UI save/edit cycle
Resolve agent partition from registry instead of hardcoding "default"
Add comprehensive integration tests for sweep config compilation
Improve debugging with extensive logging throughout sweep config flow

Diagram Walkthrough

flowchart LR
  UI["UI Form<br/>criteria_to_rules"]
  DB["Database<br/>target_criteria"]
  Registry["AgentRegistry<br/>partition lookup"]
  Compiler["SweepCompiler<br/>load_sweep_groups"]
  ConfigServer["ConfigServer<br/>get_config"]
  Agent["Agent<br/>receives config"]
  
  UI -->|rules_to_criteria| DB
  DB -->|criteria_to_rules| UI
  Registry -->|partition_id| Compiler
  Compiler -->|compiled groups| ConfigServer
  ConfigServer -->|sweep config| Agent

File Walkthrough

Relevant files

Enhancement

1 files

sweep_compiler.ex
Add debug logging to sweep group loading +14/-0

Bug fix

2 files

agent_config_generator.ex `Resolve partition from agent registry dynamically`	+43/-5
index.ex `Fix criteria parsing and add debug logging`	+58/-3

Tests

3 files

agent_config_generator_test.exs `Add partition resolution and sweep config tests`	+229/-0
sweep_targeting_integration_test.exs `Add comprehensive sweep targeting integration tests`	+509/-0
networks_live_criteria_test.exs `Add targeting rules round-trip persistence tests`	+263/-0

Documentation

3 files

proposal.md `Document sweep targeting rules bug analysis`	+100/-0
spec.md `Add sweep config requirements and scenarios`	+120/-0
tasks.md `Document investigation and fix tasks`	+102/-0

Imported from GitHub pull request. Original GitHub pull request: #2357 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2357 Original created: 2026-01-18T20:19:18Z Original updated: 2026-01-18T20:29:28Z Original head: carverauto/serviceradar:bug/agent-not-getting-sweep-config Original base: staging Original merged: 2026-01-18T20:29:27Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Tests, Enhancement ___ ### **Description** - Fix sweep group targeting rules not persisting through UI save/edit cycle - Resolve agent partition from registry instead of hardcoding "default" - Add comprehensive integration tests for sweep config compilation - Improve debugging with extensive logging throughout sweep config flow ___ ### Diagram Walkthrough ```mermaid flowchart LR UI["UI Form criteria_to_rules"] DB["Database target_criteria"] Registry["AgentRegistry partition lookup"] Compiler["SweepCompiler load_sweep_groups"] ConfigServer["ConfigServer get_config"] Agent["Agent receives config"] UI -->|rules_to_criteria| DB DB -->|criteria_to_rules| UI Registry -->|partition_id| Compiler Compiler -->|compiled groups| ConfigServer ConfigServer -->|sweep config| Agent ``` <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>1 files</summary><table> <tr> <td>sweep_compiler.ex<dd><code>Add debug logging to sweep group loading</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-fd107fcbfd91022cd5377ad79bcce1796630a25c17386187f4fbf90b35f2c941">+14/-0</a>    </td> </tr> </table></details></td></tr><tr><td>Bug fix</td><td><details><summary>2 files</summary><table> <tr> <td>agent_config_generator.ex<dd><code>Resolve partition from agent registry dynamically</code>                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-f368b9b41fa062759f00ff6fcae314cc5a42bb1caca82a9069103a803df1f9d7">+43/-5</a>    </td> </tr> <tr> <td>index.ex<dd><code>Fix criteria parsing and add debug logging</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4">+58/-3</a>    </td> </tr> </table></details></td></tr><tr><td>Tests</td><td><details><summary>3 files</summary><table> <tr> <td>agent_config_generator_test.exs<dd><code>Add partition resolution and sweep config tests</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-521bf6cc2cbd0dd5e954a9ebad2dc776c91f5339ff28e157e506aaf511e243f1">+229/-0</a>  </td> </tr> <tr> <td>sweep_targeting_integration_test.exs<dd><code>Add comprehensive sweep targeting integration tests</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-265073f265a16c79834923145b577b3b8e8617b6ab142388c320f4e642ca5931">+509/-0</a>  </td> </tr> <tr> <td>networks_live_criteria_test.exs<dd><code>Add targeting rules round-trip persistence tests</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-9f833cc5da80bc6a39537ee3584739bbc89389fbe8db5a69296670f0460b358b">+263/-0</a>  </td> </tr> </table></details></td></tr><tr><td>Documentation</td><td><details><summary>3 files</summary><table> <tr> <td>proposal.md<dd><code>Document sweep targeting rules bug analysis</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-100e57486199d4306af0bb7d1c56b21e0d23849b6ab706b3b07b0cb5a50bb41b">+100/-0</a>  </td> </tr> <tr> <td>spec.md<dd><code>Add sweep config requirements and scenarios</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-64bc0a78f0d7b670f6b43b1e3c0b6c92e992d7f2f1566eff4893c327aec5bee0">+120/-0</a>  </td> </tr> <tr> <td>tasks.md<dd><code>Document investigation and fix tasks</code>                                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2357/files#diff-fa364c50e746f7898fff4150f5c4f173ed9877b1e4c70b03fef98c3597c5257f">+102/-0</a>  </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-01-18 20:19:52 +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/2357#issuecomment-3765704858
Original created: 2026-01-18T20:19:52Z

ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive data in logs Description: New `Logger.debug/Logger.warning` statements log potentially sensitive operational data (e.g., `group.target_criteria`, `static_targets`, agent IDs, partitions, and resolved IP/target details) using `inspect/1`, which could expose internal network ranges, device targeting rules, and identifiers in application logs (also present in `elixir/serviceradar_core/lib/serviceradar/agent_config/compilers/sweep_compiler.ex` lines 132-146 and `elixir/serviceradar_core/lib/serviceradar/edge/agent_config_generator.ex` lines 299-351). index.ex [166-2390] Referred Code require Logger case load_sweep_group(socket.assigns.current_scope, id) do nil -> socket \|> put_flash(:error, "Sweep group not found") \|> push_navigate(to: ~p"/settings/networks") group -> Logger.debug( "[NetworksLive] edit_group - group.target_criteria: #{inspect(group.target_criteria)}" ) scope = socket.assigns.current_scope ash_form = Form.for_update(group, :update, domain: ServiceRadar.SweepJobs, scope: scope) rules = criteria_to_rules(group.target_criteria \|\| %{}) Logger.debug("[NetworksLive] edit_group - converted rules: #{inspect(rules)}") socket \|> assign(:page_title, "Edit Sweep Group") \|> assign(:show_form, :edit_group) ... (clipped 2204 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
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: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing user context: New logs around sweep group edits/saves do not include an acting user identifier (or equivalent audit context), reducing the ability to reliably reconstruct who performed the critical configuration change. Referred Code def handle_event("save_group", %{"form" => params}, socket) do scope = socket.assigns.current_scope # Convert criteria rules to target_criteria map target_criteria = rules_to_criteria(socket.assigns.criteria_rules) require Logger Logger.debug( "[NetworksLive] save_group - criteria_rules: #{inspect(socket.assigns.criteria_rules)}" ) Logger.debug("[NetworksLive] save_group - target_criteria: #{inspect(target_criteria)}") params = params \|> Map.put("target_criteria", target_criteria) \|> normalize_static_targets() Logger.debug("[NetworksLive] save_group - params with target_criteria: #{inspect(params)}") ash_form = ... (clipped 38 lines) 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: Sensitive data in logs: Newly added debug logs `inspect/1` full `target_criteria` and full submitted `params`, which can include network targets/IP ranges and other sensitive operational data, and are emitted as unstructured strings. Referred Code Logger.debug( "[NetworksLive] save_group - criteria_rules: #{inspect(socket.assigns.criteria_rules)}" ) Logger.debug("[NetworksLive] save_group - target_criteria: #{inspect(target_criteria)}") params = params \|> Map.put("target_criteria", target_criteria) \|> normalize_static_targets() Logger.debug("[NetworksLive] save_group - params with target_criteria: #{inspect(params)}") ash_form = socket.assigns.ash_form \|> Form.validate(params) case Form.submit(ash_form, params: params) do {:ok, group} -> Logger.debug( "[NetworksLive] save_group SUCCESS - saved group.target_criteria: #{inspect(group.target_criteria)}" ... (clipped 1 lines) 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: Registry trust boundary: `get_agent_partition/1` trusts `AgentRegistry.lookup/1` metadata (`partition_id`) without visible validation/authorization checks in the diff, which may be safe if the registry is internal-only but cannot be confirmed from the shown changes. Referred Code defp get_agent_partition(agent_id) do case AgentRegistry.lookup(agent_id) do [{_pid, metadata}] -> partition = metadata[:partition_id] \|\| "default" Logger.debug("AgentConfigGenerator: resolved partition=#{partition} for agent #{agent_id}") partition [] -> Logger.debug( "AgentConfigGenerator: agent #{agent_id} not found in registry, using partition=default" ) "default" other -> Logger.warning( "AgentConfigGenerator: unexpected registry lookup result for #{agent_id}: #{inspect(other)}" ) "default" end Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

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

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2357#issuecomment-3765704858 Original created: 2026-01-18T20:19:52Z --- <pre>ⓘ Your approaching your monthly quota for Qodo. <a href="https://www.qodo.ai/pricing">Upgrade your plan</a></pre> ## 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 data in logs </summary> Description: New <code>Logger.debug/Logger.warning</code> statements log potentially sensitive operational data (e.g., <code>group.target_criteria</code>, <code>static_targets</code>, agent IDs, partitions, and resolved IP/target details) using <code>inspect/1</code>, which could expose internal network ranges, device targeting rules, and identifiers in application logs (also present in <code>elixir/serviceradar_core/lib/serviceradar/agent_config/compilers/sweep_compiler.ex</code> lines 132-146 and <code>elixir/serviceradar_core/lib/serviceradar/edge/agent_config_generator.ex</code> lines 299-351). <a href='https://github.com/carverauto/serviceradar/pull/2357/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R166-R2390'>index.ex [166-2390]</a> <details open><summary>Referred Code</summary> ```elixir require Logger case load_sweep_group(socket.assigns.current_scope, id) do nil -> socket |> put_flash(:error, "Sweep group not found") |> push_navigate(to: ~p"/settings/networks") group -> Logger.debug( "[NetworksLive] edit_group - group.target_criteria: #{inspect(group.target_criteria)}" ) scope = socket.assigns.current_scope ash_form = Form.for_update(group, :update, domain: ServiceRadar.SweepJobs, scope: scope) rules = criteria_to_rules(group.target_criteria || %{}) Logger.debug("[NetworksLive] edit_group - converted rules: #{inspect(rules)}") socket |> assign(:page_title, "Edit Sweep Group") |> assign(:show_form, :edit_group) ... (clipped 2204 lines) ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </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=3>🟢</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> <details><summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **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> <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:** 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: 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/2357/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R325-R383'>Missing user context</a>: New logs around sweep group edits/saves do not include an acting user identifier (or equivalent audit context), reducing the ability to reliably reconstruct who performed the critical configuration change. <details open><summary>Referred Code</summary> ```elixir def handle_event("save_group", %{"form" => params}, socket) do scope = socket.assigns.current_scope # Convert criteria rules to target_criteria map target_criteria = rules_to_criteria(socket.assigns.criteria_rules) require Logger Logger.debug( "[NetworksLive] save_group - criteria_rules: #{inspect(socket.assigns.criteria_rules)}" ) Logger.debug("[NetworksLive] save_group - target_criteria: #{inspect(target_criteria)}") params = params |> Map.put("target_criteria", target_criteria) |> normalize_static_targets() Logger.debug("[NetworksLive] save_group - params with target_criteria: #{inspect(params)}") ash_form = ... (clipped 38 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 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/2357/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R332-R353'>Sensitive data in logs</a>: Newly added debug logs <code>inspect/1</code> full <code>target_criteria</code> and full submitted <code>params</code>, which can include network targets/IP ranges and other sensitive operational data, and are emitted as unstructured strings. <details open><summary>Referred Code</summary> ```elixir Logger.debug( "[NetworksLive] save_group - criteria_rules: #{inspect(socket.assigns.criteria_rules)}" ) Logger.debug("[NetworksLive] save_group - target_criteria: #{inspect(target_criteria)}") params = params |> Map.put("target_criteria", target_criteria) |> normalize_static_targets() Logger.debug("[NetworksLive] save_group - params with target_criteria: #{inspect(params)}") ash_form = socket.assigns.ash_form |> Form.validate(params) case Form.submit(ash_form, params: params) do {:ok, group} -> Logger.debug( "[NetworksLive] save_group SUCCESS - saved group.target_criteria: #{inspect(group.target_criteria)}" ... (clipped 1 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 rowspan=1>⚪</td> <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/2357/files#diff-f368b9b41fa062759f00ff6fcae314cc5a42bb1caca82a9069103a803df1f9d7R331-R351'>Registry trust boundary</a>: <code>get_agent_partition/1</code> trusts <code>AgentRegistry.lookup/1</code> metadata (<code>partition_id</code>) without visible validation/authorization checks in the diff, which may be safe if the registry is internal-only but cannot be confirmed from the shown changes. <details open><summary>Referred Code</summary> ```elixir defp get_agent_partition(agent_id) do case AgentRegistry.lookup(agent_id) do [{_pid, metadata}] -> partition = metadata[:partition_id] || "default" Logger.debug("AgentConfigGenerator: resolved partition=#{partition} for agent #{agent_id}") partition [] -> Logger.debug( "AgentConfigGenerator: agent #{agent_id} not found in registry, using partition=default" ) "default" other -> Logger.warning( "AgentConfigGenerator: unexpected registry lookup result for #{agent_id}: #{inspect(other)}" ) "default" 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 align="center" colspan="2">   </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-18 20:21:19 +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/2357#issuecomment-3765706226
Original created: 2026-01-18T20:21:19Z

ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Invalidate cache in setup In `sweep_targeting_integration_test.exs`, add `ConfigServer.invalidate(:sweep)` to the `setup` block to ensure test isolation and prevent failures due to stale cached configurations. elixir/serviceradar_core/test/serviceradar/sweep_jobs/sweep_targeting_integration_test.exs [27-37] `setup do actor = %{ id: Ash.UUID.generate(), email: "sweep-targeting-test@serviceradar.local", role: :admin } unique_id = System.unique_integer([:positive]) ++ ConfigServer.invalidate(:sweep) {:ok, actor: actor, unique_id: unique_id} end` Apply / Chat Suggestion importance[1-10]: 8 __ Why: This is a critical improvement for test stability. Adding `ConfigServer.invalidate(:sweep)` to the `setup` block ensures each test runs in isolation, preventing flaky failures caused by cached data from previous tests.	Medium
Possible issue	Improve agent partition lookup robustness In `get_agent_partition`, modify the `case` statement to handle multiple agent registrations by changing the pattern match from `[{_pid, metadata}]` to `[{_pid,` `metadata} \| _]`. elixir/serviceradar_core/lib/serviceradar/edge/agent_config_generator.ex [331-352] defp get_agent_partition(agent_id) do case AgentRegistry.lookup(agent_id) do - [{_pid, metadata}] -> + [{_pid, metadata} \| _] -> partition = metadata[:partition_id] \|\| "default" Logger.debug("AgentConfigGenerator: resolved partition=#{partition} for agent #{agent_id}") partition [] -> Logger.debug( "AgentConfigGenerator: agent #{agent_id} not found in registry, using partition=default" ) "default" other -> Logger.warning( "AgentConfigGenerator: unexpected registry lookup result for #{agent_id}: #{inspect(other)}" ) "default" end end Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential edge case where multiple agent registrations would cause the partition lookup to fail, and proposes a robust fix by handling a non-empty list instead of a single-element list.	Low
General	Align UI with backend validation In `criteria_to_rules/1`, remove the logic for handling multiple operators per field (`map_size(spec) > 1`) to align with backend validation, which rejects this format. web-ng/lib/serviceradar_web_ng_web/live/settings/networks_live/index.ex [2350-2393] defp criteria_to_rules(criteria) when is_map(criteria) do require Logger Enum.flat_map(criteria, fn {field, operator_spec} -> Logger.debug( "[NetworksLive] criteria_to_rules - parsing field=#{inspect(field)}, spec=#{inspect(operator_spec)}" ) case operator_spec do # Standard case: map with single operator %{} = spec when map_size(spec) == 1 -> [{operator, value}] = Map.to_list(spec) [ %{ id: System.unique_integer([:positive]), field: field, operator: operator, value: format_rule_value(operator, value) } ] - # Handle case where operator_spec might have multiple operators - # Create a rule for each operator - %{} = spec when map_size(spec) > 1 -> - Enum.map(Map.to_list(spec), fn {operator, value} -> - %{ - id: System.unique_integer([:positive]), - field: field, - operator: operator, - value: format_rule_value(operator, value) - } - end) - - # Handle unexpected formats + # Handle unexpected formats, including multiple operators which are not supported by backend validation other -> Logger.warning( "[NetworksLive] criteria_to_rules - unexpected operator_spec format: #{inspect(other)} for field #{field}" ) [] end end) end Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the PR adds UI logic for a multi-operator case that is explicitly rejected by backend validation, and proposes removing it to prevent an inconsistent and confusing user experience.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2357#issuecomment-3765706226 Original created: 2026-01-18T20:21:19Z --- <pre>ⓘ Your approaching your monthly quota for Qodo. <a href="https://www.qodo.ai/pricing">Upgrade your plan</a></pre> ## 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=2>Possible issue</td> <td> <details><summary>Invalidate cache in setup</summary> ___ **In <code>sweep_targeting_integration_test.exs</code>, add <code>ConfigServer.invalidate(:sweep)</code> to the <code>setup</code> block to ensure test isolation and prevent failures due to stale cached configurations.** [elixir/serviceradar_core/test/serviceradar/sweep_jobs/sweep_targeting_integration_test.exs [27-37]](https://github.com/carverauto/serviceradar/pull/2357/files#diff-265073f265a16c79834923145b577b3b8e8617b6ab142388c320f4e642ca5931R27-R37) ```diff setup do actor = %{ id: Ash.UUID.generate(), email: "sweep-targeting-test@serviceradar.local", role: :admin } unique_id = System.unique_integer([:positive]) ++ ConfigServer.invalidate(:sweep) {:ok, actor: actor, unique_id: unique_id} end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This is a critical improvement for test stability. Adding `ConfigServer.invalidate(:sweep)` to the `setup` block ensures each test runs in isolation, preventing flaky failures caused by cached data from previous tests. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Improve agent partition lookup robustness</summary> ___ **In <code>get_agent_partition</code>, modify the <code>case</code> statement to handle multiple agent registrations by changing the pattern match from <code>[{_pid, metadata}]</code> to <code>[{_pid, </code> <code>metadata} | _]</code>.** [elixir/serviceradar_core/lib/serviceradar/edge/agent_config_generator.ex [331-352]](https://github.com/carverauto/serviceradar/pull/2357/files#diff-f368b9b41fa062759f00ff6fcae314cc5a42bb1caca82a9069103a803df1f9d7R331-R352) ```diff defp get_agent_partition(agent_id) do case AgentRegistry.lookup(agent_id) do - [{_pid, metadata}] -> + [{_pid, metadata} | _] -> partition = metadata[:partition_id] || "default" Logger.debug("AgentConfigGenerator: resolved partition=#{partition} for agent #{agent_id}") partition [] -> Logger.debug( "AgentConfigGenerator: agent #{agent_id} not found in registry, using partition=default" ) "default" other -> Logger.warning( "AgentConfigGenerator: unexpected registry lookup result for #{agent_id}: #{inspect(other)}" ) "default" end end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a potential edge case where multiple agent registrations would cause the partition lookup to fail, and proposes a robust fix by handling a non-empty list instead of a single-element list. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Align UI with backend validation</summary> ___ **In <code>criteria_to_rules/1</code>, remove the logic for handling multiple operators per field (<code>map_size(spec) > 1</code>) to align with backend validation, which rejects this format.** [web-ng/lib/serviceradar_web_ng_web/live/settings/networks_live/index.ex [2350-2393]](https://github.com/carverauto/serviceradar/pull/2357/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R2350-R2393) ```diff defp criteria_to_rules(criteria) when is_map(criteria) do require Logger Enum.flat_map(criteria, fn {field, operator_spec} -> Logger.debug( "[NetworksLive] criteria_to_rules - parsing field=#{inspect(field)}, spec=#{inspect(operator_spec)}" ) case operator_spec do # Standard case: map with single operator %{} = spec when map_size(spec) == 1 -> [{operator, value}] = Map.to_list(spec) [ %{ id: System.unique_integer([:positive]), field: field, operator: operator, value: format_rule_value(operator, value) } ] - # Handle case where operator_spec might have multiple operators - # Create a rule for each operator - %{} = spec when map_size(spec) > 1 -> - Enum.map(Map.to_list(spec), fn {operator, value} -> - %{ - id: System.unique_integer([:positive]), - field: field, - operator: operator, - value: format_rule_value(operator, value) - } - end) - - # Handle unexpected formats + # Handle unexpected formats, including multiple operators which are not supported by backend validation other -> Logger.warning( "[NetworksLive] criteria_to_rules - unexpected operator_spec format: #{inspect(other)} for field #{field}" ) [] end end) end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that the PR adds UI logic for a multi-operator case that is explicitly rejected by backend validation, and proposes removing it to prevent an inconsistent and confusing user experience. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>