diff --git a/app/services/draft_notes/publish_service.rb b/app/services/draft_notes/publish_service.rb index 9e1e381c568c57f65992977b75edfc1e097703df..a7a2ad63c1cd8363fb1206746801d25d089af060 100644 --- a/app/services/draft_notes/publish_service.rb +++ b/app/services/draft_notes/publish_service.rb @@ -49,6 +49,7 @@ def publish_draft_notes notification_service.async.new_review(review) MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request) GraphqlTriggers.merge_request_merge_status_updated(merge_request) + after_publish(review) end def create_note_from_draft(draft, skip_capture_diff_note_position: false, skip_keep_around_commits: false, skip_merge_status_trigger: false) @@ -108,5 +109,11 @@ def keep_around_commits(notes) project.repository.keep_around(*shas) end end + + def after_publish(review) + # Overridden in EE + end end end + +DraftNotes::PublishService.prepend_mod diff --git a/ee/app/models/gitlab_subscriptions/features.rb b/ee/app/models/gitlab_subscriptions/features.rb index a33726775eb0863bf180507676c261e02264444f..9a290d1c6e5e56d54d2a4f82b2ba793e5f59f75b 100644 --- a/ee/app/models/gitlab_subscriptions/features.rb +++ b/ee/app/models/gitlab_subscriptions/features.rb @@ -253,6 +253,7 @@ class Features summarize_mr_changes summarize_my_mr_code_review summarize_notes + summarize_submitted_review stale_runner_cleanup_for_namespace status_page suggested_reviewers diff --git a/ee/app/models/merge_request/review_llm_summary.rb b/ee/app/models/merge_request/review_llm_summary.rb index b96ea604bc4b85d455938618d526c45da29a1bf7..3480730975e0a872e69d3653a5bba67df2075e57 100644 --- a/ee/app/models/merge_request/review_llm_summary.rb +++ b/ee/app/models/merge_request/review_llm_summary.rb @@ -9,7 +9,7 @@ class MergeRequest::ReviewLlmSummary < ApplicationRecord validates :provider, presence: true validates :content, presence: true, length: { maximum: 2056 } - enum provider: { open_ai: 0 } + enum provider: { open_ai: 0, vertex_ai: 1 } def reviewer review.author diff --git a/ee/app/policies/ee/merge_request_policy.rb b/ee/app/policies/ee/merge_request_policy.rb index 7c6e09e8cbe5a9edaf93ed44906275ae739e63d1..d96ce648cc02b35edb6ac834cde319ffb219fad0 100644 --- a/ee/app/policies/ee/merge_request_policy.rb +++ b/ee/app/policies/ee/merge_request_policy.rb @@ -46,6 +46,18 @@ module MergeRequestPolicy @subject.target_project.licensed_feature_available?(:report_approver_rules) end + with_scope :subject + condition(:ai_features_enabled) do + ::Feature.enabled?(:openai_experimentation) + end + + with_scope :subject + condition(:summarize_submitted_review_enabled) do + ::Feature.enabled?(:automatically_summarize_mr_review, subject.project) && + subject.project.licensed_feature_available?(:summarize_submitted_review) && + ::Gitlab::Llm::StageCheck.available?(subject.project.root_ancestor, :summarize_submitted_review) + end + def read_only? @subject.target_project&.namespace&.read_only? end @@ -83,6 +95,10 @@ def group_access?(protected_branch) end rule { approval_rules_licence_enabled }.enable :create_merge_request_approval_rules + + rule do + ai_features_enabled & summarize_submitted_review_enabled & can?(:read_merge_request) + end.enable :summarize_submitted_review end private diff --git a/ee/app/services/ee/draft_notes/publish_service.rb b/ee/app/services/ee/draft_notes/publish_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..803079c79ec858ff81610db829eab152a285c04e --- /dev/null +++ b/ee/app/services/ee/draft_notes/publish_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module EE + module DraftNotes + module PublishService + def after_publish(review) + Llm::SummarizeSubmittedReviewService + .new( + current_user, + merge_request, + review_id: review.id, + diff_id: merge_request.latest_merge_request_diff_id + ) + .execute + end + end + end +end diff --git a/ee/app/services/llm/summarize_submitted_review_service.rb b/ee/app/services/llm/summarize_submitted_review_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..6d3ad305659be413a88aece994440f6f6619dd90 --- /dev/null +++ b/ee/app/services/llm/summarize_submitted_review_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Llm + class SummarizeSubmittedReviewService < ::Llm::BaseService + private + + def perform + worker_perform(user, resource, :summarize_submitted_review, options) + end + + def valid? + super && + resource.to_ability_name == "merge_request" && + Ability.allowed?(user, :summarize_submitted_review, resource) + end + end +end diff --git a/ee/config/feature_flags/development/automatically_summarize_mr_review.yml b/ee/config/feature_flags/development/automatically_summarize_mr_review.yml new file mode 100644 index 0000000000000000000000000000000000000000..9f8c10f6e3bfeaadba03d438986c6a56a8a34160 --- /dev/null +++ b/ee/config/feature_flags/development/automatically_summarize_mr_review.yml @@ -0,0 +1,8 @@ +--- +name: automatically_summarize_mr_review +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124581 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/416706 +milestone: '16.2' +type: development +group: group::code review +default_enabled: false diff --git a/ee/lib/gitlab/llm/completions_factory.rb b/ee/lib/gitlab/llm/completions_factory.rb index 37f100453b9ca2c4b3728a4d898c8d38a5dc2cfb..82720e86ffc6eabd78ac3f09002dc0999417a4f5 100644 --- a/ee/lib/gitlab/llm/completions_factory.rb +++ b/ee/lib/gitlab/llm/completions_factory.rb @@ -51,6 +51,10 @@ class CompletionsFactory fill_in_merge_request_template: { service_class: ::Gitlab::Llm::VertexAi::Completions::FillInMergeRequestTemplate, prompt_class: ::Gitlab::Llm::Templates::FillInMergeRequestTemplate + }, + summarize_submitted_review: { + service_class: ::Gitlab::Llm::VertexAi::Completions::SummarizeSubmittedReview, + prompt_class: ::Gitlab::Llm::Templates::SummarizeSubmittedReview } }.freeze diff --git a/ee/lib/gitlab/llm/stage_check.rb b/ee/lib/gitlab/llm/stage_check.rb index 37467bea2c56c63207f9209da2673a1af3a61612..bbdbfa5335068b753836a3101df7280a50733277 100644 --- a/ee/lib/gitlab/llm/stage_check.rb +++ b/ee/lib/gitlab/llm/stage_check.rb @@ -14,7 +14,8 @@ class StageCheck :explain_vulnerability, :generate_commit_message, :chat, - :fill_in_merge_request_template + :fill_in_merge_request_template, + :summarize_submitted_review ].freeze BETA_FEATURES = [].freeze THIRD_PARTY_FEATURES = EXPERIMENTAL_FEATURES + BETA_FEATURES diff --git a/ee/lib/gitlab/llm/templates/summarize_submitted_review.rb b/ee/lib/gitlab/llm/templates/summarize_submitted_review.rb new file mode 100644 index 0000000000000000000000000000000000000000..f2d88f45be83c8c2e70423abc0ee634d24e45322 --- /dev/null +++ b/ee/lib/gitlab/llm/templates/summarize_submitted_review.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +module Gitlab + module Llm + module Templates + class SummarizeSubmittedReview + # We're using the chat model for this. + # Based on https://cloud.google.com/vertex-ai/docs/generative-ai/learn/models#foundation_models, + # the input token limit is 4,096 tokens. + MODEL_INPUT_TOKEN_LIMIT = 4096 + + # Based on https://cloud.google.com/vertex-ai/docs/generative-ai/learn/models#chat_model_parameters, + # a token is approximately 4 characters. Since we also need to take the + # prompt we send into consideration, we reduce the limit by a bit and only + # take 95% of the model input token limit. + INPUT_CONTENT_LIMIT = (MODEL_INPUT_TOKEN_LIMIT * 0.95) * 4 + + def initialize(review) + @review = review + end + + def to_prompt + <<-PROMPT + You are a sophisticated code review assistant. + You are acting as the reviewer for this merge request and MUST respond in first person as if you reviewed it and should always use 'I'. + You are provided with the corresponding code comment. + Use this information to create an overall summary which MUST mention the types of comments left, a comment can be either: question or recommendation. + This summary MUST NOT be longer than 3 sentences. + This summary MUST give an indication of the topics the review covered. + The summary MUST be written in present simple tense and MUST be as concise as possible. + The summary MUST also include an estimate of the overall work needed, using any of the following: "small amount of work, decent amount or significant work required" but the comment MUST make sure to note this is only an estimate, for example, "I estimate there is...". + + Code review comments: + #{review_content} + PROMPT + end + + private + + attr_reader :review + + def review_content + return unless review.present? + + content = [] + + review.notes.each do |note| + note_line = "Comment: #{note.note}\n\n" + + content << note_line if (content.length + note_line.length) < INPUT_CONTENT_LIMIT + end + + content.join("\n") + end + end + end + end +end diff --git a/ee/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review.rb b/ee/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review.rb new file mode 100644 index 0000000000000000000000000000000000000000..b1c110b913aa630e67de7de3abbaef121854cb93 --- /dev/null +++ b/ee/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Gitlab + module Llm + module VertexAi + module Completions + class SummarizeSubmittedReview < Gitlab::Llm::Completions::Base + # rubocop:disable CodeReuse/ActiveRecord + def execute(user, merge_request, options) + review = merge_request.reviews.find_by(id: options[:review_id]) + mr_diff = merge_request.merge_request_diffs.find_by(id: options[:diff_id]) + + return unless review.present? && mr_diff.present? + + response = response_for(user, review) + response_modifier = ::Gitlab::Llm::VertexAi::ResponseModifiers::Predictions.new(response) + + store_response(response_modifier, review, mr_diff) + end + # rubocop:enable CodeReuse/ActiveRecord + + private + + def response_for(user, review) + template = ai_prompt_class.new(review) + request(user, template) + end + + def request(user, template) + ::Gitlab::Llm::VertexAi::Client + .new(user) + .chat(content: template.to_prompt) + end + + def store_response(response_modifier, review, mr_diff) + return if response_modifier.errors.any? + + ::MergeRequest::ReviewLlmSummary.create!( + review: review, + merge_request_diff: mr_diff, + user: User.llm_bot, + provider: MergeRequest::ReviewLlmSummary.providers[:vertex_ai], + content: response_modifier.response_body + ) + end + end + end + end + end +end diff --git a/ee/spec/lib/gitlab/llm/templates/summarize_submitted_review_spec.rb b/ee/spec/lib/gitlab/llm/templates/summarize_submitted_review_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b243813fed011bd96a39d41729c37ff888d140e3 --- /dev/null +++ b/ee/spec/lib/gitlab/llm/templates/summarize_submitted_review_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Llm::Templates::SummarizeSubmittedReview, feature_category: :code_review_workflow do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let_it_be(:review) { create(:review, merge_request: merge_request, author: merge_request.author) } + let_it_be(:note_1) { create(:note_on_merge_request, project: project, noteable: merge_request, review: review) } + let_it_be(:note_2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, review: review) } + + subject { described_class.new(review) } + + describe '#to_prompt' do + it 'includes lines per note' do + prompt = subject.to_prompt + + expect(prompt).to include("Comment: #{note_1.note}") + expect(prompt).to include("Comment: #{note_2.note}") + end + end +end diff --git a/ee/spec/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review_spec.rb b/ee/spec/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..957faca3d125fefd45ed63302850fa1e5f8423bf --- /dev/null +++ b/ee/spec/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Llm::VertexAi::Completions::SummarizeSubmittedReview, feature_category: :code_review_workflow do + let_it_be(:llm_bot) { create(:user, :llm_bot) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let_it_be(:mr_diff) { merge_request.merge_request_diff } + let_it_be(:user) { merge_request.author } + let_it_be(:review) { create(:review, merge_request: merge_request, author: user) } + let_it_be(:note_1) { create(:note_on_merge_request, project: project, noteable: merge_request, review: review) } + let_it_be(:note_2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, review: review) } + + let(:prompt_class) { Gitlab::Llm::Templates::SummarizeSubmittedReview } + let(:options) do + { + diff_id: mr_diff.id, + review_id: review.id, + request_id: 'uuid' + } + end + + subject { described_class.new(prompt_class, options) } + + describe '#execute' do + context 'when the chat client returns a successful response' do + let(:example_answer) { 'Summary generated by AI' } + + let(:example_response) do + { + "predictions" => [ + { + "candidates" => [ + { + "author" => "", + "content" => example_answer + } + ], + "safetyAttributes" => { + "categories" => ["Violent"], + "scores" => [0.4000000059604645], + "blocked" => false + } + } + ] + } + end + + before do + allow_next_instance_of(Gitlab::Llm::VertexAi::Client) do |client| + allow(client).to receive(:chat).and_return(example_response.to_json) + end + end + + it 'stores the content from the AI response' do + expect { subject.execute(user, merge_request, options) } + .to change { mr_diff.merge_request_review_llm_summaries.count } + .by(1) + + latest_review_llm_summary = mr_diff.merge_request_review_llm_summaries.last + + aggregate_failures do + expect(latest_review_llm_summary.merge_request_diff).to eq(mr_diff) + expect(latest_review_llm_summary.review).to eq(review) + expect(latest_review_llm_summary.user).to eq(llm_bot) + expect(latest_review_llm_summary.provider).to eq('vertex_ai') + expect(latest_review_llm_summary.content).to eq('Summary generated by AI') + end + end + end + + context 'when the chat client returns an unsuccessful response' do + let(:error) { { error: { message: 'Error' } } } + + before do + allow_next_instance_of(Gitlab::Llm::VertexAi::Client) do |client| + allow(client).to receive(:chat).and_return(error.to_json) + end + end + + it 'does not store the content' do + expect { subject.execute(user, merge_request, options) } + .not_to change { mr_diff.merge_request_review_llm_summaries.count } + end + end + end +end diff --git a/ee/spec/policies/merge_request_policy_spec.rb b/ee/spec/policies/merge_request_policy_spec.rb index 250f0b7bdf3d55ba2c06e4101f6cd5181fbe5891..8f4a7051ceb02e83126512f083f9d4a0acddcd9d 100644 --- a/ee/spec/policies/merge_request_policy_spec.rb +++ b/ee/spec/policies/merge_request_policy_spec.rb @@ -469,4 +469,83 @@ def policy_for(user) end end end + + describe 'summarize_submitted_review policy', :saas do + let_it_be(:namespace) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be(:project) { create(:project, group: namespace) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let_it_be(:current_user) { developer } + + subject { described_class.new(current_user, merge_request) } + + before do + stub_ee_application_setting(should_check_namespace_plan: true) + + stub_licensed_features( + summarize_submitted_review: true, + ai_features: true + ) + + stub_feature_flags( + openai_experimentation: true, + automatically_summarize_mr_review: true + ) + + namespace.namespace_settings.update!( + experiment_features_enabled: true, + third_party_ai_features_enabled: true + ) + end + + it { is_expected.to be_allowed(:summarize_submitted_review) } + + context 'when global AI feature flag is disabled' do + before do + stub_feature_flags(openai_experimentation: false) + end + + it { is_expected.to be_disallowed(:summarize_submitted_review) } + end + + context 'when automatically_summarize_mr_review feature flag is disabled' do + before do + stub_feature_flags( + openai_experimentation: true, + automatically_summarize_mr_review: false + ) + end + + it { is_expected.to be_disallowed(:summarize_submitted_review) } + end + + context 'when license is not set' do + before do + stub_licensed_features(summarize_submitted_review: false) + end + + it { is_expected.to be_disallowed(:summarize_submitted_review) } + end + + context 'when experiment features are disabled' do + before do + namespace.namespace_settings.update!(experiment_features_enabled: false) + end + + it { is_expected.to be_disallowed(:summarize_submitted_review) } + end + + context 'when third party ai features are disabled' do + before do + namespace.namespace_settings.update!(third_party_ai_features_enabled: false) + end + + it { is_expected.to be_disallowed(:summarize_submitted_review) } + end + + context 'when user cannot read merge request' do + let(:current_user) { guest } + + it { is_expected.to be_disallowed(:summarize_submitted_review) } + end + end end diff --git a/ee/spec/services/ee/draft_notes/publish_service_spec.rb b/ee/spec/services/ee/draft_notes/publish_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..5299f35135ae86521b14f42c5b2afa3b15ba6891 --- /dev/null +++ b/ee/spec/services/ee/draft_notes/publish_service_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe DraftNotes::PublishService, feature_category: :code_review_workflow do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.author } + + def publish + DraftNotes::PublishService.new(merge_request, user).execute + end + + before do + create(:draft_note_on_text_diff, merge_request: merge_request, author: user) + end + + it 'executes Llm::SummarizeSubmittedReviewService and returns success' do + expect_next_instance_of( + Llm::SummarizeSubmittedReviewService, + user, + merge_request, + review_id: kind_of(Numeric), + diff_id: merge_request.latest_merge_request_diff_id + ) do |svc| + expect(svc).to receive(:execute) + end + + expect(publish[:status]).to eq(:success) + end +end diff --git a/ee/spec/services/llm/summarize_submitted_review_service_spec.rb b/ee/spec/services/llm/summarize_submitted_review_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..bfe78f446ca4749cb45320d4b33f0547b4f2bccb --- /dev/null +++ b/ee/spec/services/llm/summarize_submitted_review_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Llm::SummarizeSubmittedReviewService, feature_category: :code_review_workflow do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group, :public) } + let_it_be(:project) { create(:project, :public, group: group) } + let_it_be(:resource) { create(:merge_request, source_project: project, target_project: project, author: user) } + + let(:summarize_submitted_review_enabled) { true } + let(:current_user) { user } + let(:options) { {} } + + describe '#perform' do + before_all do + group.add_guest(user) + end + + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability) + .to receive(:allowed?) + .with(user, :summarize_submitted_review, resource) + .and_return(summarize_submitted_review_enabled) + end + + subject { described_class.new(current_user, resource, options).execute } + + it_behaves_like 'completion worker sync and async' do + subject { described_class.new(current_user, resource, options) } + + let(:action_name) { :summarize_submitted_review } + let(:content) { 'Summarize submitted review' } + end + + context 'when user is not member of project group' do + let(:current_user) { create(:user) } + + it { is_expected.to be_error.and have_attributes(message: eq(described_class::INVALID_MESSAGE)) } + end + + context 'when general feature flag is disabled' do + before do + stub_feature_flags(openai_experimentation: false) + end + + it { is_expected.to be_error.and have_attributes(message: eq(described_class::INVALID_MESSAGE)) } + end + + context 'when resource is not a merge_request' do + let(:resource) { create(:epic, group: group) } + + it { is_expected.to be_error.and have_attributes(message: eq(described_class::INVALID_MESSAGE)) } + end + + context 'when user has no ability to summarize_submitted_review' do + let(:summarize_submitted_review_enabled) { false } + + it { is_expected.to be_error.and have_attributes(message: eq(described_class::INVALID_MESSAGE)) } + end + end +end