diff --git a/app/models/automated_plan_reviewer.rb b/app/models/automated_plan_reviewer.rb index b2e8b2eb..fd4233df 100644 --- a/app/models/automated_plan_reviewer.rb +++ b/app/models/automated_plan_reviewer.rb @@ -1,5 +1,6 @@ class AutomatedPlanReviewer < ApplicationRecord ACTOR_TYPE = "cloud_persona" + AI_PROVIDERS = %w[openai anthropic].freeze DEFAULT_REVIEWERS = [ { key: "security-reviewer", name: "Security Reviewer", prompt_file: "prompts/reviewers/security.md", @@ -19,8 +20,9 @@ class AutomatedPlanReviewer < ApplicationRecord validates :key, uniqueness: { scope: :organization_id } validates :name, presence: true validates :prompt_text, presence: true - validates :ai_provider, presence: true + validates :ai_provider, presence: true, inclusion: { in: AI_PROVIDERS } validates :ai_model, presence: true + validate :validate_trigger_statuses scope :enabled, -> { where(enabled: true) } @@ -46,4 +48,15 @@ def self.ransackable_associations(auth_object = nil) def triggers_on_status?(status) trigger_statuses.include?(status.to_s) end + + private + + def validate_trigger_statuses + return if trigger_statuses.blank? + + invalid = trigger_statuses - Plan::STATUSES + if invalid.any? + errors.add(:trigger_statuses, "contains invalid status: #{invalid.join(', ')}. Valid statuses are: #{Plan::STATUSES.join(', ')}") + end + end end diff --git a/test/models/automated_plan_reviewer_test.rb b/test/models/automated_plan_reviewer_test.rb index cace309d..489e928c 100644 --- a/test/models/automated_plan_reviewer_test.rb +++ b/test/models/automated_plan_reviewer_test.rb @@ -94,11 +94,45 @@ class AutomatedPlanReviewerTest < ActiveSupport::TestCase assert_not enabled.include?(automated_plan_reviewers(:disabled_reviewer)) end + test "validates ai_provider inclusion" do + reviewer = automated_plan_reviewers(:security_reviewer) + reviewer.ai_provider = "unknown-provider" + assert_not reviewer.valid? + assert_includes reviewer.errors[:ai_provider], "is not included in the list" + end + + test "accepts valid ai_providers" do + reviewer = automated_plan_reviewers(:security_reviewer) + AutomatedPlanReviewer::AI_PROVIDERS.each do |provider| + reviewer.ai_provider = provider + assert reviewer.valid?, "Expected #{provider} to be valid" + end + end + test "defaults ai_provider to openai" do reviewer = AutomatedPlanReviewer.new assert_equal "openai", reviewer.ai_provider end + test "validates trigger_statuses against Plan::STATUSES" do + reviewer = automated_plan_reviewers(:security_reviewer) + reviewer.trigger_statuses = [ "considering", "invalid-status" ] + assert_not reviewer.valid? + assert reviewer.errors[:trigger_statuses].any? { |e| e.include?("invalid-status") } + end + + test "accepts valid trigger_statuses" do + reviewer = automated_plan_reviewers(:security_reviewer) + reviewer.trigger_statuses = Plan::STATUSES.dup + assert reviewer.valid? + end + + test "accepts empty trigger_statuses" do + reviewer = automated_plan_reviewers(:security_reviewer) + reviewer.trigger_statuses = [] + assert reviewer.valid? + end + test "defaults trigger_statuses to empty array" do reviewer = AutomatedPlanReviewer.new assert_equal [], reviewer.trigger_statuses