From 035d2858f58fd64cc2c0c114965bdfdbedcf8064 Mon Sep 17 00:00:00 2001 From: ramalau <71857041+ramalau0@users.noreply.github.com> Date: Mon, 27 Apr 2026 15:47:32 +0200 Subject: [PATCH] fix(agent-bots): destroy permissibles on AgentBot deletion and skip orphans in index (#14273) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit \`GET /platform/api/v1/agent_bots\` returns 500 when any \`AgentBot\` that was previously registered with a Platform App has since been deleted. The bug was introduced by a missing \`dependent: :destroy\` on the \`AgentBot\` model — deleting a bot left orphaned rows in \`platform_app_permissibles\`, which the index action later iterated over and crashed rendering with a \`NoMethodError\` on \`nil\`. Closes #13407 ## Root cause The index action loads all \`platform_app_permissibles\` for the platform app and passes each \`resource.permissible\` (the associated \`AgentBot\`) to a Jbuilder partial. When the \`AgentBot\` no longer exists, \`resource.permissible\` returns \`nil\` and the partial crashes calling \`.id\`, \`.name\`, etc. on it. Every other \`AgentBot\` association (\`agent_bot_inboxes\`, \`messages\`, \`assigned_conversations\`) had a \`dependent:\` option — \`platform_app_permissibles\` was the only one missing it. There was also an N+1 query: the index fired a separate SQL query per permissible to load each bot. ## What changed **1. Model — prevent orphans at deletion time** \`\`\`ruby has_many :platform_app_permissibles, as: :permissible, dependent: :destroy \`\`\` **2. Controller — eager-load to eliminate N+1** \`\`\`ruby @resources = @platform_app.platform_app_permissibles .where(permissible_type: 'AgentBot') .includes(:permissible) \`\`\` **3. Jbuilder — defensive nil guard for pre-existing orphans** \`\`\`ruby bot = resource.permissible next if bot.nil? json.partial! '...', resource: bot \`\`\` ## Trade-offs considered | Option | Decision | |---|---| | Rescue \`NoMethodError\` in jbuilder | Hides the failure rather than fixing it. Rejected. | | Only add the nil guard, skip the model fix | Leaves the data integrity gap open — future deletions continue creating orphans. Rejected. | | Both layers (chosen) | Model fix prevents new orphans; nil guard is defence-in-depth for any orphans that survived before deployment. | | \`dependent: :nullify\` | Doesn't apply — a nullified permissible would still cause the same nil dereference. Rejected. | ## How to reproduce 1. Create an AgentBot via the Platform API 2. Delete the AgentBot via any path (admin UI, API, or direct model call) 3. Call \`GET /platform/api/v1/agent_bots\` with a Platform App token 4. Observe 500 After this fix, the endpoint returns 200 with an empty array. Co-authored-by: Ramalau Debeila --- .../platform/api/v1/agent_bots_controller.rb | 2 +- app/models/agent_bot.rb | 1 + .../platform/api/v1/agent_bots/index.json.jbuilder | 5 ++++- .../platform/api/v1/agent_bots_controller_spec.rb | 13 +++++++++++++ spec/models/agent_bot_spec.rb | 8 ++++++++ 5 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/controllers/platform/api/v1/agent_bots_controller.rb b/app/controllers/platform/api/v1/agent_bots_controller.rb index dd70a1ba5e9..594bb856e6f 100644 --- a/app/controllers/platform/api/v1/agent_bots_controller.rb +++ b/app/controllers/platform/api/v1/agent_bots_controller.rb @@ -3,7 +3,7 @@ class Platform::Api::V1::AgentBotsController < PlatformController before_action :validate_platform_app_permissible, except: [:index, :create] def index - @resources = @platform_app.platform_app_permissibles.where(permissible_type: 'AgentBot').all + @resources = @platform_app.platform_app_permissibles.where(permissible_type: 'AgentBot').includes(:permissible) end def show; end diff --git a/app/models/agent_bot.rb b/app/models/agent_bot.rb index 63f71615d30..1649a1e2b58 100644 --- a/app/models/agent_bot.rb +++ b/app/models/agent_bot.rb @@ -32,6 +32,7 @@ class AgentBot < ApplicationRecord has_many :agent_bot_inboxes, dependent: :destroy_async has_many :inboxes, through: :agent_bot_inboxes has_many :messages, as: :sender, dependent: :nullify + has_many :platform_app_permissibles, as: :permissible, dependent: :destroy has_many :assigned_conversations, class_name: 'Conversation', foreign_key: :assignee_agent_bot_id, dependent: :nullify, diff --git a/app/views/platform/api/v1/agent_bots/index.json.jbuilder b/app/views/platform/api/v1/agent_bots/index.json.jbuilder index daa54aa2bd6..c9172d5f82b 100644 --- a/app/views/platform/api/v1/agent_bots/index.json.jbuilder +++ b/app/views/platform/api/v1/agent_bots/index.json.jbuilder @@ -1,3 +1,6 @@ json.array! @resources do |resource| - json.partial! 'platform/api/v1/models/agent_bot', formats: [:json], resource: resource.permissible + bot = resource.permissible + next if bot.nil? + + json.partial! 'platform/api/v1/models/agent_bot', formats: [:json], resource: bot end diff --git a/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb b/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb index d8a2479eb38..793439a4341 100644 --- a/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb +++ b/spec/controllers/platform/api/v1/agent_bots_controller_spec.rb @@ -39,6 +39,19 @@ RSpec.describe 'Platform Agent Bot API', type: :request do expect(data.length).to eq(1) expect(data.first['outgoing_url']).to eq(agent_bot.outgoing_url) end + + it 'returns 200 and skips orphaned permissibles when an agent bot has been deleted' do + create(:platform_app_permissible, platform_app: platform_app, permissible: agent_bot) + # Use delete (not destroy!) to bypass dependent: :destroy callbacks so the + # permissible row survives — exactly the orphan scenario described in the issue. + agent_bot.delete + + get '/platform/api/v1/agent_bots', + headers: { api_access_token: platform_app.access_token.token }, as: :json + + expect(response).to have_http_status(:success) + expect(response.parsed_body).to be_empty + end end end diff --git a/spec/models/agent_bot_spec.rb b/spec/models/agent_bot_spec.rb index d3e5e2d6485..c3cf9a409f8 100644 --- a/spec/models/agent_bot_spec.rb +++ b/spec/models/agent_bot_spec.rb @@ -6,6 +6,7 @@ RSpec.describe AgentBot do describe 'associations' do it { is_expected.to have_many(:agent_bot_inboxes) } it { is_expected.to have_many(:inboxes) } + it { is_expected.to have_many(:platform_app_permissibles) } end describe 'concerns' do @@ -38,6 +39,13 @@ RSpec.describe AgentBot do expect(message.reload.sender).to be_nil end + + it 'destroys associated platform_app_permissibles' do + platform_app = create(:platform_app) + create(:platform_app_permissible, platform_app: platform_app, permissible: agent_bot) + + expect { agent_bot.destroy! }.to change(PlatformAppPermissible, :count).by(-1) + end end describe '#system_bot?' do