carverauto/serviceradar

Fork 0

Chore/bazel build fixes core elx #2674

Merged

mfreeman451 merged 4 commits from refs/pull/2674/head into staging

2026-01-15 04:04:15 +00:00

mfreeman451 commented

2026-01-15 03:51:53 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2311
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2311
Original created: 2026-01-15T03:51:53Z
Original updated: 2026-01-15T04:04:16Z
Original head: carverauto/serviceradar:chore/bazel_build_Fixes-core-elx
Original base: staging
Original merged: 2026-01-15T04:04:15Z 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, Enhancement

Description

Create tenant membership automatically for newly registered users
Fix tenant schema configuration in Ash resources
Improve magic link email tenant resolution with fallback logic
Remove unused Bazel dependencies and npm configuration
Add idempotent migration guards for SNMP profiles

Diagram Walkthrough

flowchart LR
  A["User Registration"] --> B["AssignDefaultTenant"]
  B --> C["AssignFirstUserRole"]
  C --> D["CreateTenantMembership"]
  D --> E["User Created with Membership"]
  F["Magic Link Email"] --> G["Resolve Tenant"]
  G --> H["Fallback to Default Tenant"]
  H --> I["Send Email with Correct URL"]
  J["Ash Resources"] --> K["Add Public Schema"]
  K --> L["Fix Database Configuration"]

File Walkthrough

Relevant files

Enhancement

3 files

create_tenant_membership.ex `New module for automatic tenant membership creation`	+111/-0
user.ex `Integrate CreateTenantMembership into user creation actions`	+3/-0
scope.ex `Improve active tenant resolution and add tenant fetching`	+42/-10

Bug fix

7 files

send_magic_link_email.ex `Improve tenant resolution with multiple fallback strategies`	+92/-1
tenant.ex `Add explicit public schema configuration`	+1/-0
tenant_membership.ex `Add explicit public schema configuration`	+1/-0
nats_operator.ex `Add explicit public schema configuration`	+1/-0
nats_platform_token.ex `Add explicit public schema configuration`	+1/-0
nats_service_account.ex `Add explicit public schema configuration`	+1/-0
20260114074955_add_snmp_profiles.exs `Add idempotent guards to migration operations`	+4/-4

Configuration changes

3 files

MODULE.bazel `Remove unused Go and npm dependencies`	+2/-52
BUILD.bazel `Add snmp_service.go to build sources`	+1/-0
BUILD.bazel `Remove unused backoff dependency`	+0/-1

Imported from GitHub pull request. Original GitHub pull request: #2311 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2311 Original created: 2026-01-15T03:51:53Z Original updated: 2026-01-15T04:04:16Z Original head: carverauto/serviceradar:chore/bazel_build_Fixes-core-elx Original base: staging Original merged: 2026-01-15T04:04:15Z 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, Enhancement ___ ### **Description** - Create tenant membership automatically for newly registered users - Fix tenant schema configuration in Ash resources - Improve magic link email tenant resolution with fallback logic - Remove unused Bazel dependencies and npm configuration - Add idempotent migration guards for SNMP profiles ___ ### Diagram Walkthrough ```mermaid flowchart LR A["User Registration"] --> B["AssignDefaultTenant"] B --> C["AssignFirstUserRole"] C --> D["CreateTenantMembership"] D --> E["User Created with Membership"] F["Magic Link Email"] --> G["Resolve Tenant"] G --> H["Fallback to Default Tenant"] H --> I["Send Email with Correct URL"] J["Ash Resources"] --> K["Add Public Schema"] K --> L["Fix Database Configuration"] ``` <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>3 files</summary><table> <tr> <td>create_tenant_membership.ex<dd><code>New module for automatic tenant membership creation</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-f8b7f98fa1c3316442089ecde2bdfd5becacab32d89a5f06e61bed83989a9c56">+111/-0</a>  </td> </tr> <tr> <td>user.ex<dd><code>Integrate CreateTenantMembership into user creation actions</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-c9420769195b92ae85f0cf0f4eed8872c342663a45bf0a1f5ae3864a68d632e7">+3/-0</a>      </td> </tr> <tr> <td>scope.ex<dd><code>Improve active tenant resolution and add tenant fetching</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-8142f5d0719ccc41761eee96fa01036051e73b305766b3bb09e0c36945ef84dd">+42/-10</a>  </td> </tr> </table></details></td></tr><tr><td>Bug fix</td><td><details><summary>7 files</summary><table> <tr> <td>send_magic_link_email.ex<dd><code>Improve tenant resolution with multiple fallback strategies</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-6fee42baf4eb3b1ffd6ad280a913ddf1298485b91c8b5d8ed7cb54b6e261dc4e">+92/-1</a>    </td> </tr> <tr> <td>tenant.ex<dd><code>Add explicit public schema configuration</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-9d0658a5118ece5eac7a6326788fdf59407a52f87c4b9c9ac69e6900bc04dc2a">+1/-0</a>      </td> </tr> <tr> <td>tenant_membership.ex<dd><code>Add explicit public schema configuration</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-ca364fc0ff0f6acd8031829499723d0a07400541b19d9623144b3123c6bf1445">+1/-0</a>      </td> </tr> <tr> <td>nats_operator.ex<dd><code>Add explicit public schema configuration</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-f89214d61d9399a9344478b85a655ada729ff7128361030cebf1f6f031178624">+1/-0</a>      </td> </tr> <tr> <td>nats_platform_token.ex<dd><code>Add explicit public schema configuration</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-06c6db334f81778084d004b13b512b501e2c43a9ffca4d0ab1071c4b672b3cf4">+1/-0</a>      </td> </tr> <tr> <td>nats_service_account.ex<dd><code>Add explicit public schema configuration</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-fe5e4abb31dc089cc536a16949d1a2883e479de215ffb862ee22f27246fc1222">+1/-0</a>      </td> </tr> <tr> <td>20260114074955_add_snmp_profiles.exs<dd><code>Add idempotent guards to migration operations</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-4f3619b4ea9997d925d720374e4c4853c17ef9eacaa250690652550db711abd6">+4/-4</a>      </td> </tr> </table></details></td></tr><tr><td>Configuration changes</td><td><details><summary>3 files</summary><table> <tr> <td>MODULE.bazel<dd><code>Remove unused Go and npm dependencies</code>                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdc">+2/-52</a>    </td> </tr> <tr> <td>BUILD.bazel<dd><code>Add snmp_service.go to build sources</code>                                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-9763df8132fa8d8919489fdfaf2434921b436714eb2aa276dca0ea4f92c02ec5">+1/-0</a>      </td> </tr> <tr> <td>BUILD.bazel<dd><code>Remove unused backoff dependency</code>                                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2311/files#diff-3e838afce9a84935e04b7ff8fd3e48d5452c21538a3ea1d36e3fd00aa3c30cd0">+0/-1</a>      </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-01-15 03:52:38 +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/2311#issuecomment-3752793875
Original created: 2026-01-15T03:52:38Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Privilege escalation Description: The user registration actions accept the `:role` attribute and the new `CreateTenantMembership` change grants `:owner` membership when `user.role` is `:admin`/`:super_admin`, which could enable a privilege escalation (self-assigning elevated roles leading to tenant owner membership) if role assignment is not strictly constrained by policy/changeset validation. user.ex [97-126] Referred Code create :create do description "Register a new user with email only (magic link flow)" accept [:email, :display_name, :tenant_id, :role] change ServiceRadar.Identity.Changes.AssignDefaultTenant change ServiceRadar.Identity.Changes.AssignFirstUserRole change ServiceRadar.Identity.Changes.CreateTenantMembership primary? true end create :sign_in_with_magic_link do description "Sign in or register a user with magic link." argument :token, :string do description "The token from the magic link that was sent to the user" allow_nil? false end argument :remember_me, :boolean do description "Whether to generate a remember me token" allow_nil? true end ... (clipped 9 lines)
	Authorization bypass Description: `resolve_active_tenant/2` can fall back to `fetch_tenant_by_id/1` using a caller-provided `active_tenant_id` even when the tenant is not present in the user's memberships, and `fetch_tenant_by_id/1` reads using a platform actor, which may allow cross-tenant data exposure (e.g., retrieving another tenant’s `id/slug/name/is_platform_tenant`) and potentially improper tenant scoping if downstream logic trusts `active_tenant`. scope.ex [121-147] Referred Code defp resolve_active_tenant(user_with_data, active_tenant_id) do membership_tenant = if active_tenant_id do find_tenant_by_id(user_with_data.memberships, active_tenant_id) end membership_tenant \|\| user_with_data.tenant \|\| fetch_tenant_by_id(active_tenant_id \|\| user_with_data.tenant_id) end defp fetch_tenant_by_id(tenant_id) when is_binary(tenant_id) do actor = SystemActor.platform(:scope) Tenant \|> Ash.Query.for_read(:read) \|> Ash.Query.filter(id == ^tenant_id) \|> Ash.Query.select([:id, :slug, :name, :is_platform_tenant]) \|> Ash.read_one(actor: actor) \|> case do {:ok, %Tenant{} = tenant} -> tenant ... (clipped 6 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: 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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: 🏷️ Errors are swallowed: Membership creation failures are logged but always return `:ok`, preventing callers from detecting or handling a partial failure (user created without membership). Referred Code `{:error, %Ash.Error.Invalid{errors: errors}} -> if unique_constraint_error?(errors) do Logger.debug( "[CreateTenantMembership] Membership already exists for user #{user.id} in tenant #{tenant_id}" ) :ok else Logger.error( "[CreateTenantMembership] Failed to create membership: #{inspect(errors)}" ) :ok end {:error, reason} -> Logger.error("[CreateTenantMembership] Failed to create membership: #{inspect(reason)}") # Don't fail user creation if membership fails - user can be added manually :ok 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: 🏷️ Non-audit logging: Tenant membership creation (a permission-relevant action) is only written via `Logger` and not clearly emitted to a dedicated/structured audit trail with consistent context and outcome fields. Referred Code `{:ok, _membership} -> Logger.info( "[CreateTenantMembership] Created #{membership_role} membership for user #{user.id} in tenant #{tenant_id}" ) :ok {:error, %Ash.Error.Invalid{errors: errors}} -> if unique_constraint_error?(errors) do Logger.debug( "[CreateTenantMembership] Membership already exists for user #{user.id} in tenant #{tenant_id}" ) :ok else Logger.error( "[CreateTenantMembership] Failed to create membership: #{inspect(errors)}" ) :ok end ... (clipped 5 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: 🏷️ Potential user-facing raise: The sender raises `"Unable to resolve tenant for magic link URL"`, which may propagate to user-facing surfaces depending on caller handling and could leak internal routing details. Referred Code `nil -> Logger.warning("Unable to resolve tenant for magic link URL; falling back to base URL") base_uri _ -> raise "Unable to resolve tenant for magic link URL" 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: 🏷️ Sensitive data in logs: Logging includes `user.id`, `tenant_id`, and `inspect(errors)`/`inspect(reason)` which may contain sensitive internal details or user data depending on Ash error contents. Referred Code Logger.warning( "[CreateTenantMembership] User #{user.id} has no tenant_id, skipping membership creation" ) :ok else membership_role = membership_role_for_user(user) case create_membership(user.id, tenant_id, membership_role) do {:ok, _membership} -> Logger.info( "[CreateTenantMembership] Created #{membership_role} membership for user #{user.id} in tenant #{tenant_id}" ) :ok {:error, %Ash.Error.Invalid{errors: errors}} -> if unique_constraint_error?(errors) do Logger.debug( "[CreateTenantMembership] Membership already exists for user #{user.id} in tenant #{tenant_id}" ) ... (clipped 13 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/2311#issuecomment-3752793875 Original created: 2026-01-15T03:52:38Z --- ## 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=2>⚪</td> <td><details><summary>Privilege escalation </summary> Description: The user registration actions accept the <code>:role</code> attribute and the new <code>CreateTenantMembership</code> change grants <code>:owner</code> membership when <code>user.role</code> is <code>:admin</code>/<code>:super_admin</code>, which could enable a privilege escalation (self-assigning elevated roles leading to tenant owner membership) if role assignment is not strictly constrained by policy/changeset validation. <a href='https://github.com/carverauto/serviceradar/pull/2311/files#diff-c9420769195b92ae85f0cf0f4eed8872c342663a45bf0a1f5ae3864a68d632e7R97-R126'>user.ex [97-126]</a> <details open><summary>Referred Code</summary> ```elixir create :create do description "Register a new user with email only (magic link flow)" accept [:email, :display_name, :tenant_id, :role] change ServiceRadar.Identity.Changes.AssignDefaultTenant change ServiceRadar.Identity.Changes.AssignFirstUserRole change ServiceRadar.Identity.Changes.CreateTenantMembership primary? true end create :sign_in_with_magic_link do description "Sign in or register a user with magic link." argument :token, :string do description "The token from the magic link that was sent to the user" allow_nil? false end argument :remember_me, :boolean do description "Whether to generate a remember me token" allow_nil? true end ... (clipped 9 lines) ``` </details></details></td></tr> <tr><td><details><summary>Authorization bypass </summary> Description: <code>resolve_active_tenant/2</code> can fall back to <code>fetch_tenant_by_id/1</code> using a caller-provided <code>active_tenant_id</code> even when the tenant is not present in the user's memberships, and <code>fetch_tenant_by_id/1</code> reads using a platform actor, which may allow cross-tenant data exposure (e.g., retrieving another tenant’s <code>id/slug/name/is_platform_tenant</code>) and potentially improper tenant scoping if downstream logic trusts <code>active_tenant</code>. <a href='https://github.com/carverauto/serviceradar/pull/2311/files#diff-8142f5d0719ccc41761eee96fa01036051e73b305766b3bb09e0c36945ef84ddR121-R147'>scope.ex [121-147]</a> <details open><summary>Referred Code</summary> ```elixir defp resolve_active_tenant(user_with_data, active_tenant_id) do membership_tenant = if active_tenant_id do find_tenant_by_id(user_with_data.memberships, active_tenant_id) end membership_tenant || user_with_data.tenant || fetch_tenant_by_id(active_tenant_id || user_with_data.tenant_id) end defp fetch_tenant_by_id(tenant_id) when is_binary(tenant_id) do actor = SystemActor.platform(:scope) Tenant |> Ash.Query.for_read(:read) |> Ash.Query.filter(id == ^tenant_id) |> Ash.Query.select([:id, :slug, :name, :is_platform_tenant]) |> Ash.read_one(actor: actor) |> case do {:ok, %Tenant{} = tenant} -> tenant ... (clipped 6 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=2>🟢</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: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **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=1>🔴</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/2311/files#diff-f8b7f98fa1c3316442089ecde2bdfd5becacab32d89a5f06e61bed83989a9c56R52-R71'>Errors are swallowed</a>: Membership creation failures are logged but always return <code>:ok</code>, preventing callers from detecting or handling a partial failure (user created without membership). <details open><summary>Referred Code</summary> ```elixir {:error, %Ash.Error.Invalid{errors: errors}} -> if unique_constraint_error?(errors) do Logger.debug( "[CreateTenantMembership] Membership already exists for user #{user.id} in tenant #{tenant_id}" ) :ok else Logger.error( "[CreateTenantMembership] Failed to create membership: #{inspect(errors)}" ) :ok end {:error, reason} -> Logger.error("[CreateTenantMembership] Failed to create membership: #{inspect(reason)}") # Don't fail user creation if membership fails - user can be added manually :ok 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/2311/files#diff-f8b7f98fa1c3316442089ecde2bdfd5becacab32d89a5f06e61bed83989a9c56R45-R70'>Non-audit logging</a>: Tenant membership creation (a permission-relevant action) is only written via <code>Logger</code> and not clearly emitted to a dedicated/structured audit trail with consistent context and outcome fields. <details open><summary>Referred Code</summary> ```elixir {:ok, _membership} -> Logger.info( "[CreateTenantMembership] Created #{membership_role} membership for user #{user.id} in tenant #{tenant_id}" ) :ok {:error, %Ash.Error.Invalid{errors: errors}} -> if unique_constraint_error?(errors) do Logger.debug( "[CreateTenantMembership] Membership already exists for user #{user.id} in tenant #{tenant_id}" ) :ok else Logger.error( "[CreateTenantMembership] Failed to create membership: #{inspect(errors)}" ) :ok end ... (clipped 5 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/2311/files#diff-6fee42baf4eb3b1ffd6ad280a913ddf1298485b91c8b5d8ed7cb54b6e261dc4eR88-R94'>Potential user-facing raise</a>: The sender raises <code>"Unable to resolve tenant for magic link URL"</code>, which may propagate to user-facing surfaces depending on caller handling and could leak internal routing details. <details open><summary>Referred Code</summary> ```elixir nil -> Logger.warning("Unable to resolve tenant for magic link URL; falling back to base URL") base_uri _ -> raise "Unable to resolve tenant for magic link URL" 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/2311/files#diff-f8b7f98fa1c3316442089ecde2bdfd5becacab32d89a5f06e61bed83989a9c56R36-R69'>Sensitive data in logs</a>: Logging includes <code>user.id</code>, <code>tenant_id</code>, and <code>inspect(errors)</code>/<code>inspect(reason)</code> which may contain sensitive internal details or user data depending on Ash error contents. <details open><summary>Referred Code</summary> ```elixir Logger.warning( "[CreateTenantMembership] User #{user.id} has no tenant_id, skipping membership creation" ) :ok else membership_role = membership_role_for_user(user) case create_membership(user.id, tenant_id, membership_role) do {:ok, _membership} -> Logger.info( "[CreateTenantMembership] Created #{membership_role} membership for user #{user.id} in tenant #{tenant_id}" ) :ok {:error, %Ash.Error.Invalid{errors: errors}} -> if unique_constraint_error?(errors) do Logger.debug( "[CreateTenantMembership] Membership already exists for user #{user.id} in tenant #{tenant_id}" ) ... (clipped 13 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-15 03:53:43 +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/2311#issuecomment-3752795573
Original created: 2026-01-15T03:53:43Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	✅ ~~Use specific error types for checks~~ Suggestion Impact: The commit removed string-based matching in unique_constraint_error?/1 and switched to matching on a specific Ash error struct (Ash.Error.Changes.InvalidAttribute) and inspecting its private_vars for constraint_type: :unique, improving robustness. It did not implement the exact suggested structs (Ash.Error.Changes.Unique / AshPostgres.Error.DuplicateEntry) but follows the same intent. code diff: @@ -96,12 +101,11 @@ defp unique_constraint_error?(errors) do Enum.any?(errors, fn error -> case error do - %Ash.Error.Changes.InvalidChanges{message: msg} -> - String.contains?(to_string(msg), "unique") or - String.contains?(to_string(msg), "already exists") - - %{class: :invalid, message: msg} when is_binary(msg) -> - String.contains?(msg, "unique") or String.contains?(msg, "already exists") + # AshPostgres wraps unique constraint violations in InvalidAttribute + # with constraint_type: :unique in private_vars + %Ash.Error.Changes.InvalidAttribute{private_vars: private_vars} + when is_list(private_vars) -> + Keyword.get(private_vars, :constraint_type) == :unique _ -> false Refactor `unique_constraint_error?/1` to pattern match on specific Ash error structs instead of relying on string matching error messages for better robustness. elixir/serviceradar_core/lib/serviceradar/identity/changes/create_tenant_membership.ex [96-110] defp unique_constraint_error?(errors) do - Enum.any?(errors, fn error -> - case error do - %Ash.Error.Changes.InvalidChanges{message: msg} -> - String.contains?(to_string(msg), "unique") or - String.contains?(to_string(msg), "already exists") - - %{class: :invalid, message: msg} when is_binary(msg) -> - String.contains?(msg, "unique") or String.contains?(msg, "already exists") - - _ -> - false - end + Enum.any?(errors, fn + %Ash.Error.Changes.Unique{} -> true + %AshPostgres.Error.DuplicateEntry{} -> true + _ -> false end) end `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that string-matching on error messages is brittle. Using pattern matching on specific error types like `Ash.Error.Changes.Unique` makes the code more robust and maintainable.	Medium
Possible issue	✅ ~~Prevent membership errors aborting flow~~ Suggestion Impact: The after_action now wraps create_membership_if_needed(user) in a try/rescue and logs unexpected exceptions, preventing them from bubbling up and failing the flow (logging uses Exception.message/1). The commit also includes additional refactoring of membership creation/error handling beyond the suggestion. code diff: `Ash.Changeset.after_action(changeset, fn _changeset, user -> - create_membership_if_needed(user) + try do + create_membership_if_needed(user) + rescue + e -> + Logger.error("[CreateTenantMembership] Unexpected error: #{Exception.message(e)}") + end + {:ok, user}` Wrap the `create_membership_if_needed` call in a `try/rescue` block within the `after_action` to catch and log unexpected exceptions, preventing user creation from failing. elixir/serviceradar_core/lib/serviceradar/identity/changes/create_tenant_membership.ex [26-29] `Ash.Changeset.after_action(changeset, fn _changeset, user -> - create_membership_if_needed(user) + try do + create_membership_if_needed(user) + rescue + e -> + Logger.error("[CreateTenantMembership] Unexpected error: #{inspect(e)}") + end {:ok, user} end)` `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: This is a good defensive programming suggestion. While the `create_membership_if_needed` function already handles expected errors, a `try/rescue` block would prevent any unexpected exceptions from crashing the user creation process, making it more resilient.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2311#issuecomment-3752795573 Original created: 2026-01-15T03:53:43Z --- ## 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=1>General</td> <td> <details><summary>✅ <s>Use specific error types for checks</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit removed string-based matching in unique_constraint_error?/1 and switched to matching on a specific Ash error struct (Ash.Error.Changes.InvalidAttribute) and inspecting its private_vars for constraint_type: :unique, improving robustness. It did not implement the exact suggested structs (Ash.Error.Changes.Unique / AshPostgres.Error.DuplicateEntry) but follows the same intent. code diff: ```diff @@ -96,12 +101,11 @@ defp unique_constraint_error?(errors) do Enum.any?(errors, fn error -> case error do - %Ash.Error.Changes.InvalidChanges{message: msg} -> - String.contains?(to_string(msg), "unique") or - String.contains?(to_string(msg), "already exists") - - %{class: :invalid, message: msg} when is_binary(msg) -> - String.contains?(msg, "unique") or String.contains?(msg, "already exists") + # AshPostgres wraps unique constraint violations in InvalidAttribute + # with constraint_type: :unique in private_vars + %Ash.Error.Changes.InvalidAttribute{private_vars: private_vars} + when is_list(private_vars) -> + Keyword.get(private_vars, :constraint_type) == :unique _ -> false ``` </details> ___ **Refactor <code>unique_constraint_error?/1</code> to pattern match on specific Ash error structs instead of relying on string matching error messages for better robustness.** [elixir/serviceradar_core/lib/serviceradar/identity/changes/create_tenant_membership.ex [96-110]](https://github.com/carverauto/serviceradar/pull/2311/files#diff-f8b7f98fa1c3316442089ecde2bdfd5becacab32d89a5f06e61bed83989a9c56R96-R110) ```diff defp unique_constraint_error?(errors) do - Enum.any?(errors, fn error -> - case error do - %Ash.Error.Changes.InvalidChanges{message: msg} -> - String.contains?(to_string(msg), "unique") or - String.contains?(to_string(msg), "already exists") - - %{class: :invalid, message: msg} when is_binary(msg) -> - String.contains?(msg, "unique") or String.contains?(msg, "already exists") - - _ -> - false - end + Enum.any?(errors, fn + %Ash.Error.Changes.Unique{} -> true + %AshPostgres.Error.DuplicateEntry{} -> true + _ -> false end) end ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that string-matching on error messages is brittle. Using pattern matching on specific error types like `Ash.Error.Changes.Unique` makes the code more robust and maintainable. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>✅ <s>Prevent membership errors aborting flow</s></summary> ___ <details><summary>Suggestion Impact:</summary>The after_action now wraps create_membership_if_needed(user) in a try/rescue and logs unexpected exceptions, preventing them from bubbling up and failing the flow (logging uses Exception.message/1). The commit also includes additional refactoring of membership creation/error handling beyond the suggestion. code diff: ```diff Ash.Changeset.after_action(changeset, fn _changeset, user -> - create_membership_if_needed(user) + try do + create_membership_if_needed(user) + rescue + e -> + Logger.error("[CreateTenantMembership] Unexpected error: #{Exception.message(e)}") + end + {:ok, user} ``` </details> ___ **Wrap the <code>create_membership_if_needed</code> call in a <code>try/rescue</code> block within the <code>after_action</code> to catch and log unexpected exceptions, preventing user creation from failing.** [elixir/serviceradar_core/lib/serviceradar/identity/changes/create_tenant_membership.ex [26-29]](https://github.com/carverauto/serviceradar/pull/2311/files#diff-f8b7f98fa1c3316442089ecde2bdfd5becacab32d89a5f06e61bed83989a9c56R26-R29) ```diff Ash.Changeset.after_action(changeset, fn _changeset, user -> - create_membership_if_needed(user) + try do + create_membership_if_needed(user) + rescue + e -> + Logger.error("[CreateTenantMembership] Unexpected error: #{inspect(e)}") + end {:ok, user} end) ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: This is a good defensive programming suggestion. While the `create_membership_if_needed` function already handles expected errors, a `try/rescue` block would prevent any unexpected exceptions from crashing the user creation process, making it more resilient. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>