carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

adding dropdown for agent selection #2845

Merged
mfreeman451 merged 17 commits from refs/pull/2845/head into staging 2026-02-04 15:41:54 +00:00
Conversation 2 Commits 17 Files changed 67 +5166 -490
mfreeman451 commented 2026-02-03 23:49:45 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #2691
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2691
Original created: 2026-02-03T23:49:45Z
Original updated: 2026-02-04T15:41:56Z
Original head: carverauto/serviceradar:2651-bug-interfaces-for-switches-not-showing-up-in-device-details
Original base: staging
Original merged: 2026-02-04T15:41:54Z 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, Bug fix

Description

  • Add server-side validation for discovery job agent assignments against registered agents

  • Implement dropdown selector for agent assignment in discovery job editor UI

  • Add run-now action to trigger discovery jobs immediately from the jobs table

  • Validate agent exists in correct partition and reject invalid assignments

Diagram Walkthrough

flowchart LR
  A["Discovery Job Form"] -->|"select agent"| B["Agent Dropdown"]
  B -->|"filtered by partition"| C["Registered Agents"]
  A -->|"submit"| D["Agent Assignment Validation"]
  D -->|"verify exists & partition"| C
  D -->|"reject invalid"| E["Validation Error"]
  D -->|"accept valid"| F["Job Saved"]
  G["Run Now Button"] -->|"trigger"| H["TriggerMapperRun Change"]
  H -->|"set run_now_at"| I["Job Queued"]

File Walkthrough
Relevant files
Enhancement
4 files
trigger_mapper_run.ex
New change module to trigger mapper job runs                         
+30/-0   
mapper_job.ex
Add agent validation and run_now action                                   
+9/-0     
agent_assignment.ex
New validation for agent assignment eligibility                   
+64/-0   
index.ex
Add agent dropdown and run-now button to UI                           
+111/-8 
Tests
2 files
mapper_job_validation_test.exs
Tests for agent assignment validation logic                           
+71/-0   
networks_live_test.exs
Tests for agent selector and run-now action                           
+24/-0   
Documentation
5 files
design.md
Design document for interface visibility fix                         
+28/-0   
proposal.md
Proposal for interface tab and discovery UX improvements 
+14/-0   
spec.md
UI requirements for interface tab visibility                         
+22/-0   
spec.md
API requirements for agent validation and run-now               
+36/-0   
tasks.md
Implementation tasks and progress tracking                             
+7/-0     
Imported from GitHub pull request. Original GitHub pull request: #2691 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2691 Original created: 2026-02-03T23:49:45Z Original updated: 2026-02-04T15:41:56Z Original head: carverauto/serviceradar:2651-bug-interfaces-for-switches-not-showing-up-in-device-details Original base: staging Original merged: 2026-02-04T15:41:54Z 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, Bug fix ___ ### **Description** - Add server-side validation for discovery job agent assignments against registered agents - Implement dropdown selector for agent assignment in discovery job editor UI - Add run-now action to trigger discovery jobs immediately from the jobs table - Validate agent exists in correct partition and reject invalid assignments ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Discovery Job Form"] -->|"select agent"| B["Agent Dropdown"] B -->|"filtered by partition"| C["Registered Agents"] A -->|"submit"| D["Agent Assignment Validation"] D -->|"verify exists & partition"| C D -->|"reject invalid"| E["Validation Error"] D -->|"accept valid"| F["Job Saved"] G["Run Now Button"] -->|"trigger"| H["TriggerMapperRun Change"] H -->|"set run_now_at"| I["Job Queued"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>trigger_mapper_run.ex</strong><dd><code>New change module to trigger mapper job runs</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-a30beb6dc660d11dca7fc319b1cdceb4d872b5be7e4d1784787cfcb4a8a72b65">+30/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>mapper_job.ex</strong><dd><code>Add agent validation and run_now action</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-67a2c9ea1e7a049af23d3e1c5d8ae13352dfc9c882047abd563b5df376a29500">+9/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>agent_assignment.ex</strong><dd><code>New validation for agent assignment eligibility</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-a4f7b6f7bac9bca7ee4081580e5ea6a0b2e89a663cd12eecbdd1d784438f1910">+64/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>index.ex</strong><dd><code>Add agent dropdown and run-now button to UI</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4">+111/-8</a>&nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>mapper_job_validation_test.exs</strong><dd><code>Tests for agent assignment validation logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-06e8b84597461ea11b023f91c3ffa399777b2b6cea227961b668da8b9b57f471">+71/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>networks_live_test.exs</strong><dd><code>Tests for agent selector and run-now action</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-81736ddb52f2c677276e60e51726c3fd0fb96f6d4bd349649f0b381e28e48088">+24/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>5 files</summary><table> <tr> <td><strong>design.md</strong><dd><code>Design document for interface visibility fix</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-e78d5d7049410ded6d337b46ae9d54de28c6bc86f83e7c48eb238b78f65879bb">+28/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>proposal.md</strong><dd><code>Proposal for interface tab and discovery UX improvements</code>&nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-3c75b15e15a805a9688a5fa4914ff1d1f4f1fea38a49f59c56e942368f769559">+14/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>UI requirements for interface tab visibility</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-6ce43156eeaf66e515b659488d8c7bec4e20c594eafb24c33f2f0ab4b33117a6">+22/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>API requirements for agent validation and run-now</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-1304ff6e23482c54c5d05506f603a2e16f949dcc5bdd28c2ce7d15271f59f8b7">+36/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Implementation tasks and progress tracking</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2691/files#diff-90d0fc25507514c900790f3f58d5054bf3dc1d7624e699bad3190e0210ece1a0">+7/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2026-02-03 23:50:25 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2691#issuecomment-3844429945
Original created: 2026-02-03T23:50:25Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Authorization bypass

Description: The new LiveView event run_mapper_job and the new agent loading path (load_agents/1) may
allow any user who can access the page to enumerate registered agents and trigger
discovery jobs without an explicit role/authorization check in the LiveView layer, which
could enable unauthorized job execution or infrastructure information disclosure if
upstream authorization is misconfigured.
index.ex [361-2797]

Referred Code 
def handle_event("run_mapper_job", %{"id" => id}, socket) do
  scope = socket.assigns.current_scope

  with {:ok, job} <- fetch_mapper_job(scope, id),
       {:ok, _updated} <- Ash.update(job, %{}, action: :run_now, scope: scope) do
    {:noreply,
     socket
     |> assign(:mapper_jobs, load_mapper_jobs(scope))
     |> put_flash(:info, "Discovery job queued to run now")}
  else
    {:error, :not_found} ->
      {:noreply, put_flash(socket, :error, "Discovery job not found")}

    {:error, reason} ->
      {:noreply,
       socket
       |> put_flash(:error, "Failed to run discovery job: #{format_error(reason)}")}
  end
end

def handle_event("save_mapper_job", params, socket) do


 ... (clipped 2416 lines)
Ticket Compliance
🟡
🎫 #2651
🔴 Investigate and fix why certain switch devices are not showing expected associated
interfaces in device details (i.e., why interfaces are not being picked up/returned for
the device query example provided).
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: Security-First Input Validation and Data Handling

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

Status: Passed

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

🔴
Generic: Secure Error Handling

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

Status:
Leaky user error: The new flash message interpolates format_error(reason) into a user-facing error, which
can expose internal/system details depending on how format_error/1 renders exceptions.

Referred Code 
  {:error, reason} ->
    {:noreply,
     socket
     |> put_flash(:error, "Failed to run discovery job: #{format_error(reason)}")}
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:
Missing audit logging: The new "run now" discovery-job trigger path does not emit an audit log entry
capturing user identity, timestamp, action, and outcome, so event reconstruction may rely
on unseen infrastructure.

Referred Code 
def handle_event("run_mapper_job", %{"id" => id}, socket) do
  scope = socket.assigns.current_scope

  with {:ok, job} <- fetch_mapper_job(scope, id),
       {:ok, _updated} <- Ash.update(job, %{}, action: :run_now, scope: scope) do
    {:noreply,
     socket
     |> assign(:mapper_jobs, load_mapper_jobs(scope))
     |> put_flash(:info, "Discovery job queued to run now")}
  else
    {:error, :not_found} ->
      {:noreply, put_flash(socket, :error, "Discovery job not found")}

    {:error, reason} ->
      {:noreply,
       socket
       |> put_flash(:error, "Failed to run discovery job: #{format_error(reason)}")}
  end

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:
Swallowed lookup errors: Agent lookup failures return a generic validation error ("agent lookup failed")
without logging the underlying reason, which may hinder production diagnosis unless
upstream telemetry exists.

Referred Code 
Agent
|> Ash.Query.for_read(:by_uid, %{uid: agent_id})
|> Ash.read_one(actor: actor)
|> case do
  {:ok, %Agent{} = agent} ->
    agent_partition = Map.get(agent.metadata || %{}, "partition_id") || "default"

    if agent_partition == partition do
      :ok
    else
      {:error,
       field: :agent_id,
       message:
         "agent '#{agent_id}' belongs to partition '#{agent_partition}', not '#{partition}'"}
    end

  {:ok, nil} ->
    {:error, field: :agent_id, message: "agent '#{agent_id}' not found"}

  {:error, _reason} ->
    {:error, field: :agent_id, message: "agent lookup failed"}


 ... (clipped 1 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:
Logs inspect(reason): The new warning log line uses inspect(reason), which may include internal details or
sensitive fields depending on the error payload returned by Ash.read/2.

Referred Code 
  {:error, reason} ->
    Logger.warning("load_agents: failed to load agents - #{inspect(reason)}")
    []
end

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/2691#issuecomment-3844429945 Original created: 2026-02-03T23:50:25Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/cc7e9520eea25d0c0f054d959dbb24f64481b049 --> Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary><strong>Authorization bypass</strong></summary><br> <b>Description:</b> The new LiveView event <code>run_mapper_job</code> and the new agent loading path (<code>load_agents/1</code>) may <br>allow any user who can access the page to enumerate registered agents and trigger <br>discovery jobs without an explicit role/authorization check in the LiveView layer, which <br>could enable unauthorized job execution or infrastructure information disclosure if <br>upstream authorization is misconfigured.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2691/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R361-R2797'>index.ex [361-2797]</a></strong><br> <details open><summary>Referred Code</summary> ```elixir def handle_event("run_mapper_job", %{"id" => id}, socket) do scope = socket.assigns.current_scope with {:ok, job} <- fetch_mapper_job(scope, id), {:ok, _updated} <- Ash.update(job, %{}, action: :run_now, scope: scope) do {:noreply, socket |> assign(:mapper_jobs, load_mapper_jobs(scope)) |> put_flash(:info, "Discovery job queued to run now")} else {:error, :not_found} -> {:noreply, put_flash(socket, :error, "Discovery job not found")} {:error, reason} -> {:noreply, socket |> put_flash(:error, "Failed to run discovery job: #{format_error(reason)}")} end end def handle_event("save_mapper_job", params, socket) do ... (clipped 2416 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2651>#2651</a></summary> <table width='100%'><tbody> <tr><td rowspan=1>🔴</td> <td>Investigate and fix why certain switch devices are not showing expected associated <br>interfaces in device details (i.e., why interfaces are not being picked up/returned for <br>the device query example provided).<br></td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=2>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>🔴</td> <td><details> <summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2691/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R374-R378'><strong>Leaky user error</strong></a>: The new flash message interpolates <code>format_error(reason)</code> into a user-facing error, which <br>can expose internal/system details depending on how <code>format_error/1</code> renders exceptions.<br> <details open><summary>Referred Code</summary> ```elixir {:error, reason} -> {:noreply, socket |> put_flash(:error, "Failed to run discovery job: #{format_error(reason)}")} 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><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2691/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R361-R378'><strong>Missing audit logging</strong></a>: The new &quot;run now&quot; discovery-job trigger path does not emit an audit log entry <br>capturing user identity, timestamp, action, and outcome, so event reconstruction may rely <br>on unseen infrastructure.<br> <details open><summary>Referred Code</summary> ```elixir def handle_event("run_mapper_job", %{"id" => id}, socket) do scope = socket.assigns.current_scope with {:ok, job} <- fetch_mapper_job(scope, id), {:ok, _updated} <- Ash.update(job, %{}, action: :run_now, scope: scope) do {:noreply, socket |> assign(:mapper_jobs, load_mapper_jobs(scope)) |> put_flash(:info, "Discovery job queued to run now")} else {:error, :not_found} -> {:noreply, put_flash(socket, :error, "Discovery job not found")} {:error, reason} -> {:noreply, socket |> put_flash(:error, "Failed to run discovery job: #{format_error(reason)}")} 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><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2691/files#diff-a4f7b6f7bac9bca7ee4081580e5ea6a0b2e89a663cd12eecbdd1d784438f1910R31-R52'><strong>Swallowed lookup errors</strong></a>: Agent lookup failures return a generic validation error (<code>&quot;agent lookup failed&quot;</code>) <br>without logging the underlying reason, which may hinder production diagnosis unless <br>upstream telemetry exists.<br> <details open><summary>Referred Code</summary> ```elixir Agent |> Ash.Query.for_read(:by_uid, %{uid: agent_id}) |> Ash.read_one(actor: actor) |> case do {:ok, %Agent{} = agent} -> agent_partition = Map.get(agent.metadata || %{}, "partition_id") || "default" if agent_partition == partition do :ok else {:error, field: :agent_id, message: "agent '#{agent_id}' belongs to partition '#{agent_partition}', not '#{partition}'"} end {:ok, nil} -> {:error, field: :agent_id, message: "agent '#{agent_id}' not found"} {:error, _reason} -> {:error, field: :agent_id, message: "agent lookup failed"} ... (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><details> <summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2691/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R2730-R2733'><strong>Logs inspect(reason)</strong></a>: The new warning log line uses <code>inspect(reason)</code>, which may include internal details or <br>sensitive fields depending on the error payload returned by <code>Ash.read/2</code>.<br> <details open><summary>Referred Code</summary> ```elixir {:error, reason} -> Logger.warning("load_agents: failed to load agents - #{inspect(reason)}") [] 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"> - [ ] Update <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>
qodo-code-review[bot] commented 2026-02-03 23:51:36 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2691#issuecomment-3844433161
Original created: 2026-02-03T23:51:36Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
✅ Prevent crashes from non-stringifiable agent capabilities
Suggestion Impact:agent_supports_mapper?/1 was updated to Enum.filter capabilities for binaries/atoms before mapping to_string/1, preventing crashes on malformed capability entries.

code diff:

@@ -2752,6 +2894,7 @@
 
   defp agent_supports_mapper?(agent) do
     (agent.capabilities || [])
+    |> Enum.filter(&(is_binary(&1) || is_atom(&1)))
     |> Enum.map(&to_string/1)
     |> Enum.any?(&(&1 == "mapper"))
   end

In agent_supports_mapper?/1, filter the agent.capabilities list to include only
binaries and atoms before calling to_string/1 to prevent potential crashes from
malformed data.

web-ng/lib/serviceradar_web_ng_web/live/settings/networks_live/index.ex [2753-2757]

     defp agent_supports_mapper?(agent) do
       (agent.capabilities || [])
+      |> Enum.filter(&(is_binary(&1) || is_atom(&1)))
       |> Enum.map(&to_string/1)
       |> Enum.any?(&(&1 == "mapper"))
     end

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential crash due to unexpected data types in agent.capabilities and provides a robust fix, improving the code's resilience.

Low
General
✅ Log the reason for agent lookup failures
Suggestion Impact:Added Logger requirement and updated the {:error, reason} match arm to log the failure reason before returning the validation error.

code diff:

+  require Logger
 
   alias ServiceRadar.Actors.SystemActor
   alias ServiceRadar.Infrastructure.Agent
@@ -14,42 +16,55 @@
   @impl true
   def validate(changeset, _opts, _context) do
     agent_id =
-      Ash.Changeset.get_attribute(changeset, :agent_id) ||
-        Map.get(changeset.data, :agent_id)
+      changeset
+      |> fetch_agent_id()
+      |> normalize_agent_id()
 
-    agent_id = normalize_agent_id(agent_id)
+    case agent_id do
+      nil -> :ok
+      _ -> validate_agent_partition(agent_id, fetch_partition(changeset))
+    end
+  end
 
-    if is_nil(agent_id) do
+  defp fetch_agent_id(changeset) do
+    Ash.Changeset.get_attribute(changeset, :agent_id) ||
+      Map.get(changeset.data, :agent_id)
+  end
+
+  defp fetch_partition(changeset) do
+    Ash.Changeset.get_attribute(changeset, :partition) ||
+      Map.get(changeset.data, :partition) || "default"
+  end
+
+  defp validate_agent_partition(agent_id, partition) do
+    actor = SystemActor.system(:mapper_job_agent_validation)
+
+    Agent
+    |> Ash.Query.for_read(:by_uid, %{uid: agent_id})
+    |> Ash.read_one(actor: actor)
+    |> case do
+      {:ok, %Agent{} = agent} ->
+        compare_partitions(agent_id, partition, agent)
+
+      {:ok, nil} ->
+        {:error, field: :agent_id, message: "agent '#{agent_id}' not found"}
+
+      {:error, reason} ->
+        Logger.error("Agent lookup failed during validation: #{inspect(reason)}")
+        {:error, field: :agent_id, message: "agent lookup failed"}

In validate/3, log the error reason when the Ash.read_one call fails to improve
observability and aid in debugging system-level issues.

elixir/serviceradar_core/lib/serviceradar/network_discovery/validations/agent_assignment.ex [50-52]

     ...
-        {:error, _reason} ->
+        {:error, reason} ->
+          Logger.error("Agent lookup failed during validation: #{inspect(reason)}")
           {:error, field: :agent_id, message: "agent lookup failed"}
       end
     end
   end
   ...

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that an error reason is being ignored and proposes logging it, which improves debuggability but does not fix a functional bug.

Low
  • Update
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2691#issuecomment-3844433161 Original created: 2026-02-03T23:51:36Z --- ## PR Code Suggestions ✨ <!-- cc7e952 --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>✅ <s>Prevent crashes from non-stringifiable agent capabilities<!-- not_implemented --></s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>agent_supports_mapper?/1 was updated to Enum.filter capabilities for binaries/atoms before mapping to_string/1, preventing crashes on malformed capability entries. code diff: ```diff @@ -2752,6 +2894,7 @@ defp agent_supports_mapper?(agent) do (agent.capabilities || []) + |> Enum.filter(&(is_binary(&1) || is_atom(&1))) |> Enum.map(&to_string/1) |> Enum.any?(&(&1 == "mapper")) end ``` </details> ___ **In <code>agent_supports_mapper?/1</code>, filter the <code>agent.capabilities</code> list to include only <br>binaries and atoms before calling <code>to_string/1</code> to prevent potential crashes from <br>malformed data.** [web-ng/lib/serviceradar_web_ng_web/live/settings/networks_live/index.ex [2753-2757]](https://github.com/carverauto/serviceradar/pull/2691/files#diff-b2127e71582033bc6dfd2d7397f56bf43c1f7c613defffc504b3d8ee1e7406c4R2753-R2757) ```diff defp agent_supports_mapper?(agent) do (agent.capabilities || []) + |> Enum.filter(&(is_binary(&1) || is_atom(&1))) |> Enum.map(&to_string/1) |> Enum.any?(&(&1 == "mapper")) end ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a potential crash due to unexpected data types in `agent.capabilities` and provides a robust fix, improving the code's resilience. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>✅ <s>Log the reason for agent lookup failures<!-- not_implemented --></s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>Added Logger requirement and updated the {:error, reason} match arm to log the failure reason before returning the validation error. code diff: ```diff + require Logger alias ServiceRadar.Actors.SystemActor alias ServiceRadar.Infrastructure.Agent @@ -14,42 +16,55 @@ @impl true def validate(changeset, _opts, _context) do agent_id = - Ash.Changeset.get_attribute(changeset, :agent_id) || - Map.get(changeset.data, :agent_id) + changeset + |> fetch_agent_id() + |> normalize_agent_id() - agent_id = normalize_agent_id(agent_id) + case agent_id do + nil -> :ok + _ -> validate_agent_partition(agent_id, fetch_partition(changeset)) + end + end - if is_nil(agent_id) do + defp fetch_agent_id(changeset) do + Ash.Changeset.get_attribute(changeset, :agent_id) || + Map.get(changeset.data, :agent_id) + end + + defp fetch_partition(changeset) do + Ash.Changeset.get_attribute(changeset, :partition) || + Map.get(changeset.data, :partition) || "default" + end + + defp validate_agent_partition(agent_id, partition) do + actor = SystemActor.system(:mapper_job_agent_validation) + + Agent + |> Ash.Query.for_read(:by_uid, %{uid: agent_id}) + |> Ash.read_one(actor: actor) + |> case do + {:ok, %Agent{} = agent} -> + compare_partitions(agent_id, partition, agent) + + {:ok, nil} -> + {:error, field: :agent_id, message: "agent '#{agent_id}' not found"} + + {:error, reason} -> + Logger.error("Agent lookup failed during validation: #{inspect(reason)}") + {:error, field: :agent_id, message: "agent lookup failed"} ``` </details> ___ **In <code>validate/3</code>, log the error reason when the <code>Ash.read_one</code> call fails to improve <br>observability and aid in debugging system-level issues.** [elixir/serviceradar_core/lib/serviceradar/network_discovery/validations/agent_assignment.ex [50-52]](https://github.com/carverauto/serviceradar/pull/2691/files#diff-a4f7b6f7bac9bca7ee4081580e5ea6a0b2e89a663cd12eecbdd1d784438f1910R50-R52) ```diff ... - {:error, _reason} -> + {:error, reason} -> + Logger.error("Agent lookup failed during validation: #{inspect(reason)}") {:error, field: :agent_id, message: "agent lookup failed"} end end end ... ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies that an error reason is being ignored and proposes logging it, which improves debuggability but does not fix a functional bug. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2845
No description provided.