From b8b62ad0f11833cb221d35edc0469c8358b0e4f1 Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Thu, 25 Jun 2026 17:38:11 +0530 Subject: [PATCH] feat: Harden model override preferences (5/6) (#14846) ## Description Hardens the Captain model override preferences API so account-level overrides follow the same feature-router contract used by runtime LLM calls. The API now permits model and feature keys from `llm.yml`, removes blank model overrides, rejects invalid saved model combinations, and returns each feature's effective model, provider, and source for UI clients. Fixes https://linear.app/chatwoot/issue/CW-7425/test-new-models ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality not to work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Verified the account preferences API and account model validation behavior for valid overrides, invalid model values, unknown feature keys, blank override removal, and effective model/provider/source payload metadata. - `eval "$(rbenv init -)" && bundle exec rspec spec/controllers/api/v1/accounts/captain/preferences_controller_spec.rb spec/models/account_spec.rb spec/models/concerns/captain_featurable_spec.rb spec/lib/llm/feature_router_spec.rb` - `eval "$(rbenv init -)" && bundle exec rubocop app/controllers/api/v1/accounts/captain/preferences_controller.rb app/models/concerns/account_settings_schema.rb app/models/concerns/captain_featurable.rb spec/controllers/api/v1/accounts/captain/preferences_controller_spec.rb spec/models/account_spec.rb spec/models/concerns/captain_featurable_spec.rb spec/lib/llm/feature_router_spec.rb` - `git diff --check` ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- .../captain/preferences_controller.rb | 13 ++- .../super_admin/accounts_controller.rb | 3 +- app/dashboards/account_dashboard.rb | 5 +- app/models/concerns/captain_featurable.rb | 19 +++- config/locales/en.yml | 22 +++++ .../fields/captain_model_overrides_field.rb | 56 +++++++++++ .../_form.html.erb | 27 ++++++ .../_show.html.erb | 43 +++++++++ .../captain/preferences_controller_spec.rb | 59 ++++++++++++ .../super_admin/accounts_controller_spec.rb | 92 +++++++++++++++++++ spec/models/account_spec.rb | 13 +++ 11 files changed, 344 insertions(+), 8 deletions(-) create mode 100644 enterprise/app/fields/captain_model_overrides_field.rb create mode 100644 enterprise/app/views/fields/captain_model_overrides_field/_form.html.erb create mode 100644 enterprise/app/views/fields/captain_model_overrides_field/_show.html.erb diff --git a/app/controllers/api/v1/accounts/captain/preferences_controller.rb b/app/controllers/api/v1/accounts/captain/preferences_controller.rb index a62ec115c..04eeff92b 100644 --- a/app/controllers/api/v1/accounts/captain/preferences_controller.rb +++ b/app/controllers/api/v1/accounts/captain/preferences_controller.rb @@ -8,8 +8,8 @@ class Api::V1::Accounts::Captain::PreferencesController < Api::V1::Accounts::Bas def update params_to_update = captain_params - @current_account.captain_models = params_to_update[:captain_models] if params_to_update[:captain_models] - @current_account.captain_features = params_to_update[:captain_features] if params_to_update[:captain_features] + @current_account.captain_models = params_to_update[:captain_models] if params_to_update.key?(:captain_models) + @current_account.captain_features = params_to_update[:captain_features] if params_to_update.key?(:captain_features) @current_account.save! render json: preferences_payload @@ -38,7 +38,7 @@ class Api::V1::Accounts::Captain::PreferencesController < Api::V1::Accounts::Bas def merged_captain_models existing_models = @current_account.captain_models || {} - existing_models.merge(permitted_captain_models) + existing_models.merge(permitted_captain_models).compact_blank.presence end def merged_captain_features @@ -61,13 +61,16 @@ class Api::V1::Accounts::Captain::PreferencesController < Api::V1::Accounts::Bas def features_with_account_preferences preferences = Current.account.captain_preferences account_features = preferences[:features] || {} - account_models = preferences[:models] || {} Llm::Models.feature_keys.index_with do |feature_key| config = Llm::Models.feature_config(feature_key) + route = Llm::FeatureRouter.resolve(feature: feature_key, account: Current.account) config.merge( enabled: account_features[feature_key] == true, - selected: account_models[feature_key] || config[:default] + model: route[:model], + selected: route[:model], + provider: route[:provider], + source: route[:source] ) end end diff --git a/app/controllers/super_admin/accounts_controller.rb b/app/controllers/super_admin/accounts_controller.rb index 27ce587f7..59b99c37e 100644 --- a/app/controllers/super_admin/accounts_controller.rb +++ b/app/controllers/super_admin/accounts_controller.rb @@ -35,7 +35,8 @@ class SuperAdmin::AccountsController < SuperAdmin::ApplicationController # def resource_params permitted_params = super - permitted_params[:limits] = permitted_params[:limits].to_h.compact + permitted_params[:limits] = permitted_params[:limits].to_h.compact if permitted_params.key?(:limits) + permitted_params[:captain_models] = permitted_params[:captain_models].to_h.compact_blank.presence if permitted_params.key?(:captain_models) permitted_params[:selected_feature_flags] = params[:enabled_features].keys.map(&:to_sym) if params[:enabled_features].present? permitted_params end diff --git a/app/dashboards/account_dashboard.rb b/app/dashboards/account_dashboard.rb index 9be674f11..b2683f2e0 100644 --- a/app/dashboards/account_dashboard.rb +++ b/app/dashboards/account_dashboard.rb @@ -18,6 +18,7 @@ class AccountDashboard < Administrate::BaseDashboard # Add all_features last so it appears after manually_managed_features attributes[:all_features] = AccountFeaturesField + attributes[:captain_models] = CaptainModelOverridesField attributes else @@ -57,6 +58,7 @@ class AccountDashboard < Administrate::BaseDashboard attrs = %i[custom_attributes limits] attrs << :manually_managed_features if ChatwootApp.chatwoot_cloud? attrs << :all_features + attrs << :captain_models attrs else [] @@ -79,6 +81,7 @@ class AccountDashboard < Administrate::BaseDashboard attrs = %i[limits] attrs << :manually_managed_features if ChatwootApp.chatwoot_cloud? attrs << :all_features + attrs << :captain_models attrs else [] @@ -117,7 +120,7 @@ class AccountDashboard < Administrate::BaseDashboard # to prevent an error from being raised (wrong number of arguments) # Reference: https://github.com/thoughtbot/administrate/pull/2356/files#diff-4e220b661b88f9a19ac527c50d6f1577ef6ab7b0bed2bfdf048e22e6bfa74a05R204 def permitted_attributes(action) - attrs = super + [limits: {}] + attrs = super + [limits: {}, captain_models: {}] # Add manually_managed_features to permitted attributes only for Chatwoot Cloud attrs << { manually_managed_features: [] } if ChatwootApp.chatwoot_cloud? diff --git a/app/models/concerns/captain_featurable.rb b/app/models/concerns/captain_featurable.rb index 2d99dd41f..16566eb25 100644 --- a/app/models/concerns/captain_featurable.rb +++ b/app/models/concerns/captain_featurable.rb @@ -4,6 +4,7 @@ module CaptainFeaturable extend ActiveSupport::Concern included do + before_validation :normalize_captain_models validate :validate_captain_models # Dynamically define accessor methods for each captain feature @@ -46,11 +47,27 @@ module CaptainFeaturable return if captain_models.blank? captain_models.each do |feature_key, model_name| - next if model_name.blank? + unless Llm::Models.feature?(feature_key) + errors.add(:captain_models, "'#{feature_key}' is not a known feature") + next + end + next if Llm::Models.valid_model_for?(feature_key, model_name) allowed_models = Llm::Models.models_for(feature_key) errors.add(:captain_models, "'#{model_name}' is not a valid model for #{feature_key}. Allowed: #{allowed_models.join(', ')}") end end + + def normalize_captain_models + return unless captain_models.is_a?(Hash) + + normalized_models = captain_models.each_with_object({}) do |(feature_key, model_name), result| + next if model_name.blank? + + result[feature_key.to_s] = model_name.to_s + end + + self.captain_models = normalized_models.presence + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index c3672ef7b..22d3630af 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -574,6 +574,28 @@ en: ssl_status: custom_domain_not_configured: 'Custom domain is not configured' super_admin: + captain_model_overrides: + form: + helper_text: 'Leave a model blank to use the YAML default for that AI feature.' + use_default: 'Use default: %{model} (%{model_id})' + show: + summary: 'View model routing' + provider: 'Provider' + model: 'Model' + sources: + account_override: 'Account override' + default: 'Default' + features: + editor: 'Editor' + assistant: 'Assistant' + copilot: 'Copilot' + label_suggestion: 'Label suggestion' + document_faq_generation: 'Document FAQ generation' + help_center_article_generation: 'Help center article generation' + onboarding_content_generation: 'Onboarding content generation' + help_center_query_translation: 'Help center query translation' + audio_transcription: 'Audio transcription' + help_center_search: 'Help center search' push_diagnostics: user_not_found: 'User not found.' no_subscriptions_to_test: 'Select at least one subscription to test.' diff --git a/enterprise/app/fields/captain_model_overrides_field.rb b/enterprise/app/fields/captain_model_overrides_field.rb new file mode 100644 index 000000000..a8f3fe399 --- /dev/null +++ b/enterprise/app/fields/captain_model_overrides_field.rb @@ -0,0 +1,56 @@ +require 'administrate/field/base' + +class CaptainModelOverridesField < Administrate::Field::Base + def feature_rows + Llm::Models.feature_keys.map do |feature_key| + route = Llm::FeatureRouter.resolve(feature: feature_key, account: resource) + + { + key: feature_key, + name: feature_name(feature_key), + provider: provider_label(route[:provider]), + provider_id: route[:provider], + model: model_label(route[:model]), + model_id: route[:model], + default_model: model_label(default_model_id(feature_key)), + default_model_id: default_model_id(feature_key), + source: route[:source], + source_label: source_label(route[:source]), + selected_override: selected_override(feature_key), + options: model_options(feature_key) + } + end + end + + private + + def selected_override(feature_key) + resource.captain_models&.[](feature_key).presence + end + + def default_model_id(feature_key) + Llm::Models.default_model_for(feature_key) + end + + def model_options(feature_key) + Llm::Models.feature_config(feature_key)[:models].map do |model| + [model[:display_name] || model[:id], model[:id]] + end + end + + def model_label(model_id) + Llm::Models.model_config(model_id)&.dig('display_name') || model_id + end + + def provider_label(provider_id) + Llm::Models.providers.dig(provider_id, 'display_name') || provider_id + end + + def feature_name(feature_key) + I18n.t("super_admin.captain_model_overrides.features.#{feature_key}", default: feature_key.humanize) + end + + def source_label(source) + I18n.t("super_admin.captain_model_overrides.sources.#{source}") + end +end diff --git a/enterprise/app/views/fields/captain_model_overrides_field/_form.html.erb b/enterprise/app/views/fields/captain_model_overrides_field/_form.html.erb new file mode 100644 index 000000000..0420ab09a --- /dev/null +++ b/enterprise/app/views/fields/captain_model_overrides_field/_form.html.erb @@ -0,0 +1,27 @@ +
+ <%= f.label field.attribute %> +
+ +
+

<%= t('super_admin.captain_model_overrides.form.helper_text') %>

+ +
+ <% field.feature_rows.each do |feature| %> +
+
+
<%= feature[:name] %>
+
<%= feature[:key] %>
+
+ + <%= select_tag( + "account[captain_models][#{feature[:key]}]", + options_for_select( + [[t('super_admin.captain_model_overrides.form.use_default', model: feature[:default_model], model_id: feature[:default_model_id]), '']] + feature[:options], + feature[:selected_override] + ), + class: 'block w-full rounded-md border-slate-300 text-sm' + ) %> +
+ <% end %> +
+
diff --git a/enterprise/app/views/fields/captain_model_overrides_field/_show.html.erb b/enterprise/app/views/fields/captain_model_overrides_field/_show.html.erb new file mode 100644 index 000000000..4215e93aa --- /dev/null +++ b/enterprise/app/views/fields/captain_model_overrides_field/_show.html.erb @@ -0,0 +1,43 @@ +
+ + <%= t('super_admin.captain_model_overrides.show.summary') %> + + + + + +
+
+ <% field.feature_rows.each do |feature| %> +
+
+
+
<%= feature[:name] %>
+
<%= feature[:key] %>
+
+ + <%= feature[:source_label] %> + +
+ +
+
+
<%= t('super_admin.captain_model_overrides.show.provider') %>
+
+ <%= feature[:provider] %> + (<%= feature[:provider_id] %>) +
+
+
+
<%= t('super_admin.captain_model_overrides.show.model') %>
+
+ <%= feature[:model] %> + (<%= feature[:model_id] %>) +
+
+
+
+ <% end %> +
+
+
diff --git a/spec/controllers/api/v1/accounts/captain/preferences_controller_spec.rb b/spec/controllers/api/v1/accounts/captain/preferences_controller_spec.rb index 82122dfa5..dfc2e4ff0 100644 --- a/spec/controllers/api/v1/accounts/captain/preferences_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/captain/preferences_controller_spec.rb @@ -45,6 +45,28 @@ RSpec.describe 'Api::V1::Accounts::Captain::Preferences', type: :request do expect(json_response).to have_key(:models) expect(json_response).to have_key(:features) end + + it 'returns effective model provider and source for each feature' do + account.update!(captain_models: { 'editor' => 'gpt-4.1' }) + + get "/api/v1/accounts/#{account.id}/captain/preferences", + headers: admin.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:success) + expect(json_response.dig(:features, :editor)).to include( + model: 'gpt-4.1', + selected: 'gpt-4.1', + provider: 'openai', + source: 'account_override' + ) + expect(json_response.dig(:features, :label_suggestion)).to include( + model: Llm::Models.default_model_for('label_suggestion'), + selected: Llm::Models.default_model_for('label_suggestion'), + provider: 'openai', + source: 'default' + ) + end end end @@ -84,6 +106,43 @@ RSpec.describe 'Api::V1::Accounts::Captain::Preferences', type: :request do expect(account.reload.captain_models['editor']).to eq('gpt-4.1-mini') end + it 'does not persist unknown captain model feature keys' do + put "/api/v1/accounts/#{account.id}/captain/preferences", + headers: admin.create_new_auth_token, + params: { captain_models: { editor: 'gpt-4.1-mini', unknown_feature: 'gpt-4.1' } }, + as: :json + + expect(response).to have_http_status(:success) + expect(account.reload.captain_models).to eq('editor' => 'gpt-4.1-mini') + end + + it 'rejects invalid captain model values for the feature' do + put "/api/v1/accounts/#{account.id}/captain/preferences", + headers: admin.create_new_auth_token, + params: { captain_models: { label_suggestion: 'gpt-5.1' } }, + as: :json + + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response[:message]).to include('not a valid model for label_suggestion') + expect(account.reload.captain_models).to be_nil + end + + it 'removes blank captain model overrides' do + account.update!(captain_models: { 'editor' => 'gpt-4.1' }) + + put "/api/v1/accounts/#{account.id}/captain/preferences", + headers: admin.create_new_auth_token, + params: { captain_models: { editor: '' } }, + as: :json + + expect(response).to have_http_status(:success) + expect(account.reload.captain_models).to be_nil + expect(json_response.dig(:features, :editor)).to include( + selected: Llm::Models.default_model_for('editor'), + source: 'default' + ) + end + it 'updates captain_models for document FAQ generation' do put "/api/v1/accounts/#{account.id}/captain/preferences", headers: admin.create_new_auth_token, diff --git a/spec/controllers/super_admin/accounts_controller_spec.rb b/spec/controllers/super_admin/accounts_controller_spec.rb index e4ff81a08..b2f4ff405 100644 --- a/spec/controllers/super_admin/accounts_controller_spec.rb +++ b/spec/controllers/super_admin/accounts_controller_spec.rb @@ -25,6 +25,98 @@ RSpec.describe 'Super Admin accounts API', type: :request do end end + describe 'GET /super_admin/accounts/{account_id}' do + context 'when it is an authenticated user' do + it 'shows effective Captain model routing', if: ChatwootApp.enterprise? do + account.update!(captain_models: { 'editor' => 'gpt-4.1' }) + sign_in(super_admin, scope: :super_admin) + + get "/super_admin/accounts/#{account.id}" + document = Nokogiri::HTML(response.body) + summaries = document.css('details summary').map { |summary| summary.text.squish } + + expect(response).to have_http_status(:success) + expect(document.at_css('#captain_models').text.squish).to eq('Captain models') + expect(summaries).to include('View model routing') + expect(summaries).not_to include('All features') + expect(summaries).not_to include('Captain models') + expect(response.body).to include('Editor', 'OpenAI', 'openai', 'gpt-4.1', 'Account override', 'Label suggestion', 'Default') + end + end + end + + describe 'GET /super_admin/accounts/{account_id}/edit' do + context 'when it is an authenticated user' do + it 'renders a Captain model selector for every AI feature', if: ChatwootApp.enterprise? do + account.update!(captain_models: { 'editor' => 'gpt-4.1' }) + sign_in(super_admin, scope: :super_admin) + + get "/super_admin/accounts/#{account.id}/edit" + + expect(response).to have_http_status(:success) + Llm::Models.feature_keys.each do |feature_key| + expect(response.body).to include("account[captain_models][#{feature_key}]") + end + + document = Nokogiri::HTML(response.body) + editor_select = document.at_css('select[name="account[captain_models][editor]"]') + default_model_id = Llm::Models.default_model_for('editor') + default_model = Llm::Models.model_config(default_model_id)['display_name'] + + expect(editor_select.at_css('option[value=""]').text.squish).to eq("Use default: #{default_model} (#{default_model_id})") + end + end + end + + describe 'PATCH /super_admin/accounts/{account_id}' do + context 'when it is an authenticated user' do + it 'updates Captain model overrides without changing unrelated settings' do + account.update!( + captain_models: { 'editor' => 'gpt-4.1' }, + keep_pending_on_bot_failure: true + ) + sign_in(super_admin, scope: :super_admin) + + patch "/super_admin/accounts/#{account.id}", + params: { + account: { + name: account.name, + locale: account.locale, + status: account.status, + captain_models: { + editor: '', + assistant: 'gpt-5.2' + } + } + } + + expect(response).to have_http_status(:redirect) + expect(account.reload.captain_models).to eq('assistant' => 'gpt-5.2') + expect(account.keep_pending_on_bot_failure).to be true + end + + it 'rejects invalid Captain model overrides' do + sign_in(super_admin, scope: :super_admin) + + patch "/super_admin/accounts/#{account.id}", + params: { + account: { + name: account.name, + locale: account.locale, + status: account.status, + captain_models: { + label_suggestion: 'gpt-5.1' + } + } + } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.body).to include('not a valid model for label_suggestion') + expect(account.reload.captain_models).to be_nil + end + end + end + describe 'POST /super_admin/accounts/{account_id}/reset_cache' do before do create(:label, account: account) diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 38ca9694a..56bd41f7c 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -385,6 +385,19 @@ RSpec.describe Account do expect(account).to be_valid end + + it 'rejects unknown feature keys' do + account.captain_models = { 'unknown_feature' => 'gpt-4.1' } + + expect(account).not_to be_valid + expect(account.errors[:captain_models]).to include("'unknown_feature' is not a known feature") + end + + it 'removes blank model overrides before saving' do + account.update!(captain_models: { 'editor' => '', 'assistant' => 'gpt-5.2' }) + + expect(account.captain_models).to eq('assistant' => 'gpt-5.2') + end end end end