From 56899c1422e0935ee84fc796ac5d5ee513c3fdca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 20 May 2024 17:45:57 +0200 Subject: [PATCH 01/32] First draft of adding MR to chat --- app/serializers/merge_request_ai_entity.rb | 17 +++ app/serializers/merge_request_serializer.rb | 2 + ee/app/models/ai/ai_resource/merge_request.rb | 31 ++++ ee/app/workers/llm/completion_worker.rb | 2 +- .../llm/chain/agents/zero_shot/executor.rb | 2 +- .../tools/merge_request_reader/executor.rb | 141 ++++++++++++++++++ .../merge_request_reader/prompts/anthropic.rb | 32 ++++ ee/lib/gitlab/llm/completions/chat.rb | 1 + .../templates/summarize_new_merge_request.rb | 32 +--- ee/lib/gitlab/llm/utils/merge_request_tool.rb | 37 +++++ 10 files changed, 270 insertions(+), 27 deletions(-) create mode 100644 app/serializers/merge_request_ai_entity.rb create mode 100644 ee/app/models/ai/ai_resource/merge_request.rb create mode 100644 ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb create mode 100644 ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb create mode 100644 ee/lib/gitlab/llm/utils/merge_request_tool.rb diff --git a/app/serializers/merge_request_ai_entity.rb b/app/serializers/merge_request_ai_entity.rb new file mode 100644 index 00000000000000..c85c0084288a03 --- /dev/null +++ b/app/serializers/merge_request_ai_entity.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class MergeRequestAiEntity < Api::Entities::MergeRequestBasic + expose :mr_comments do |_mr, options| + options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit]/2) + end + + expose :diff do |_mr, options| + Gitlab::Llm::Utils::MergeRequestTool.extract_diff( + source_project: options[:resource].source_project, + source_branch: options[:resource].source_branch, + target_project: options[:resource].target_project, + target_branch: options[:resource].target_branch, + character_limit: options[:notes_limit]/2 + ) + end +end diff --git a/app/serializers/merge_request_serializer.rb b/app/serializers/merge_request_serializer.rb index 6f83978841d973..5fe4978f76db40 100644 --- a/app/serializers/merge_request_serializer.rb +++ b/app/serializers/merge_request_serializer.rb @@ -19,6 +19,8 @@ def represent(merge_request, opts = {}, entity = nil) MergeRequestPollCachedWidgetEntity when 'poll_widget' MergeRequestPollWidgetEntity + when 'ai' + MergeRequestAiEntity else # fallback to widget for old poll requests without `serializer` set MergeRequestWidgetEntity diff --git a/ee/app/models/ai/ai_resource/merge_request.rb b/ee/app/models/ai/ai_resource/merge_request.rb new file mode 100644 index 00000000000000..081a5b25b4858f --- /dev/null +++ b/ee/app/models/ai/ai_resource/merge_request.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Ai + module AiResource + class MergeRequest < Ai::AiResource::BaseAiResource + include Ai::AiResource::Concerns::Noteable + + def serialize_for_ai(user:, content_limit:) + ::MergeRequestSerializer.new(current_user: user) # rubocop: disable CodeReuse/Serializer + .represent(resource, { + user: user, + notes_limit: content_limit, + serializer: 'ai', + resource: self + }) + end + + def current_page_sentence + <<~SENTENCE + The user is currently on a page that displays a merge request with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'MergeRequestReader' tool. + SENTENCE + end + + def current_page_short_description + <<~SENTENCE + The user is currently on a page that displays a merge request with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the merge request is '#{resource.title}'. Remember to use the 'MergeRequestReader' tool if they ask a question about the MergeRequest. + SENTENCE + end + end + end +end diff --git a/ee/app/workers/llm/completion_worker.rb b/ee/app/workers/llm/completion_worker.rb index 258fe03212dc9c..3d2dd9052b6666 100644 --- a/ee/app/workers/llm/completion_worker.rb +++ b/ee/app/workers/llm/completion_worker.rb @@ -30,7 +30,7 @@ def deserialize_message(message_hash, options) end def perform_for(message, options = {}) - perform_async(serialize_message(message), options) + perform_inline(serialize_message(message), options) end end diff --git a/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb b/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb index e4eff63854528a..c565d0ee651a5a 100644 --- a/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb +++ b/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb @@ -243,7 +243,7 @@ def source_template You have access to the following GitLab resources: %s. You also have access to all information that can be helpful to someone working in software development of any kind. - At the moment, you do not have access to the following GitLab resources: Merge Requests, Pipelines, Vulnerabilities. + At the moment, you do not have access to the following GitLab resources: Pipelines, Vulnerabilities. %s diff --git a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb new file mode 100644 index 00000000000000..dfb30af0be65b2 --- /dev/null +++ b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +module Gitlab + module Llm + module Chain + module Tools + module MergeRequestReader + class Executor < Identifier + include Concerns::ReaderTooling + + RESOURCE_NAME = 'merge request' + NAME = "MergeRequestReader" + HUMAN_NAME = 'Merge Request Search' + DESCRIPTION = 'Gets the content of the current merge request (also referenced as this or that) the user sees ' \ + 'or a specific merge request identified by an ID or a URL.' \ + 'In this context, "merge request" means part of work that is ready to be merged. ' \ + 'Action Input for this tool should be the original question or merge request identifier.' + + EXAMPLE = + <<~PROMPT + Question: Please identify the author of !123 merge request + Thought: You have access to the same resources as user who asks a question. + Question is about the content of a merge request, so you need to use "MergeRequestReader" tool to retrieve and read merge request. + Based on this information you can present final answer about merge request. + Action: MergeRequestReader + Action Input: Please identify the author of !123 merge request + PROMPT + + PROVIDER_PROMPT_CLASSES = { + ai_gateway: ::Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic, + anthropic: ::Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic + }.freeze + + PROJECT_REGEX = { + 'url' => MergeRequest.link_reference_pattern, + 'reference' => MergeRequest.reference_pattern + }.freeze + + SYSTEM_PROMPT = Utils::Prompt.as_system( + <<~PROMPT + You can fetch information about a resource called: a merge request. + A merge request can be referenced by url or numeric IDs preceded by symbol. + A merge request can also be referenced by a GitLab reference. A GitLab reference ends with a number preceded by the delimiter ! and contains one or more /. + ResourceIdentifierType can only be one of [current, iid, url, reference]. + ResourceIdentifier can be number, url. If ResourceIdentifier is not a number or a url, use "current". + When you see a GitLab reference, ResourceIdentifierType should be reference. + + Make sure the response is a valid JSON. The answer should be just the JSON without any other commentary! + References in the given question to the current issue can be also for example "this merge request" or "that merge request", + referencing the merge request that the user currently sees. + Question: (the user question) + Response (follow the exact JSON response): + ```json + { + "ResourceIdentifierType": + "ResourceIdentifier": + } + ``` + + Examples of merge request reference identifier: + + Question: The user question or request may include https://some.host.name/some/long/path/-/merge_requests/410692 + Response: + ```json + { + "ResourceIdentifierType": "url", + "ResourceIdentifier": "https://some.host.name/some/long/path/-/merge_requests/410692" + } + ``` + + Question: the user question or request may include: !12312312 + Response: + ```json + { + "ResourceIdentifierType": "iid", + "ResourceIdentifier": 12312312 + } + ``` + + Question: the user question or request may include long/groups/path!12312312 + Response: + ```json + { + "ResourceIdentifierType": "reference", + "ResourceIdentifier": "long/groups/path!12312312" + } + ``` + + Question: Summarize the current merge request + Response: + ```json + { + "ResourceIdentifierType": "current", + "ResourceIdentifier": "current" + } + ``` + + Begin! + PROMPT + ) + + PROMPT_TEMPLATE = [ + SYSTEM_PROMPT, + Utils::Prompt.as_assistant("%s"), + Utils::Prompt.as_user("Question: %s") + ].freeze + + private + + def reference_pattern_by_type + PROJECT_REGEX + end + + def by_iid(resource_identifier) + return unless projects_from_context + + mrs = MergeRequest.in_projects(projects_from_context).iid_in(resource_identifier.to_i) + + mrs.first if mrs.one? + end + + def extract_resource(text, type) + project = extract_project(text, type) + return unless project + + extractor = Gitlab::ReferenceExtractor.new(project, context.current_user) + extractor.analyze(text, {}) + mrs = extractor.merge_requests + + mrs.first if mrs.one? + end + + def resource_name + RESOURCE_NAME + end + end + end + end + end + end +end diff --git a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb new file mode 100644 index 00000000000000..5c4e152b3d4466 --- /dev/null +++ b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Gitlab + module Llm + module Chain + module Tools + module MergeRequestReader + module Prompts + class Anthropic + include Concerns::AnthropicPrompt + + def self.prompt(options) + conversation = Utils::Prompt.role_conversation([ + ::Gitlab::Llm::Chain::Tools::IssueReader::Executor::SYSTEM_PROMPT, + Utils::Prompt.as_user(options[:input]), + Utils::Prompt.as_assistant(options[:suggestions], "```json + \{ + \"ResourceIdentifierType\": \"") + ]) + + { + prompt: conversation, + options: { model: ::Gitlab::Llm::AiGateway::Client::CLAUDE_3_HAIKU } + } + end + end + end + end + end + end + end +end diff --git a/ee/lib/gitlab/llm/completions/chat.rb b/ee/lib/gitlab/llm/completions/chat.rb index 394784b0147642..e2829ee65dae07 100644 --- a/ee/lib/gitlab/llm/completions/chat.rb +++ b/ee/lib/gitlab/llm/completions/chat.rb @@ -11,6 +11,7 @@ class Chat < Base TOOLS = [ ::Gitlab::Llm::Chain::Tools::IssueReader, + ::Gitlab::Llm::Chain::Tools::MergeRequestReader, ::Gitlab::Llm::Chain::Tools::GitlabDocumentation, ::Gitlab::Llm::Chain::Tools::EpicReader, ::Gitlab::Llm::Chain::Tools::CiEditorAssistant diff --git a/ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb b/ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb index 3d9e8287d8792f..86c8f4c74b6eaa 100644 --- a/ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb +++ b/ee/lib/gitlab/llm/templates/summarize_new_merge_request.rb @@ -39,34 +39,16 @@ def to_prompt attr_reader :user, :project, :params def extracted_diff - compare = CompareService - .new(source_project, params[:source_branch]) - .execute(project, params[:target_branch]) - - return unless compare - - # Extract only the diff strings and discard everything else - compare.raw_diffs.to_a.map do |raw_diff| - # Each diff string starts with information about the lines changed, - # bracketed by @@. Removing this saves us tokens. - # - # Ex: @@ -0,0 +1,58 @@\n+# frozen_string_literal: true\n+\n+module MergeRequests\n+ - - next if raw_diff.diff.encoding != Encoding::UTF_8 || raw_diff.has_binary_notice? - - diff_output(raw_diff.old_path, raw_diff.new_path, raw_diff.diff.sub(Gitlab::Regex.git_diff_prefix, "")) - end.join.truncate_words(CHARACTER_LIMIT) + Gitlab::Llm::Utils::MergeRequestTool.extract_diff( + source_project: source_project, + source_branch: params[:source_branch], + target_project: project, + target_branch: params[:target_branch], + character_limit: CHARACTER_LIMIT + ) end strong_memoize_attr :extracted_diff - def diff_output(old_path, new_path, diff) - <<~DIFF - --- #{old_path} - +++ #{new_path} - #{diff} - DIFF - end - def source_project return project unless params[:source_project_id] diff --git a/ee/lib/gitlab/llm/utils/merge_request_tool.rb b/ee/lib/gitlab/llm/utils/merge_request_tool.rb new file mode 100644 index 00000000000000..9ea47bc39069a8 --- /dev/null +++ b/ee/lib/gitlab/llm/utils/merge_request_tool.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Gitlab + module Llm + module Utils + class MergeRequestTool + def self.extract_diff(source_project:, source_branch:, target_project:, target_branch:, character_limit:) + compare = CompareService + .new(source_project, source_branch) + .execute(target_project, target_branch) + + return unless compare + + # Extract only the diff strings and discard everything else + compare.raw_diffs.to_a.map do |raw_diff| + # Each diff string starts with information about the lines changed, + # bracketed by @@. Removing this saves us tokens. + # + # Ex: @@ -0,0 +1,58 @@\n+# frozen_string_literal: true\n+\n+module MergeRequests\n+ + + next if raw_diff.diff.encoding != Encoding::UTF_8 || raw_diff.has_binary_notice? + + diff_output(raw_diff.old_path, raw_diff.new_path, raw_diff.diff.sub(Gitlab::Regex.git_diff_prefix, "")) + end.join.truncate_words(character_limit) + end + + def diff_output(old_path, new_path, diff) + <<~DIFF + --- #{old_path} + +++ #{new_path} + #{diff} + DIFF + end + end + end + end +end -- GitLab From e238a1d36fa2f45c0897574f0aabaddc8c99b5ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 20 May 2024 20:46:53 +0200 Subject: [PATCH 02/32] Make the mr reader work --- app/serializers/merge_request_ai_entity.rb | 12 ++++++------ .../llm/chain/tools/merge_request_reader/executor.rb | 2 +- ee/lib/gitlab/llm/utils/merge_request_tool.rb | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/serializers/merge_request_ai_entity.rb b/app/serializers/merge_request_ai_entity.rb index c85c0084288a03..110d2fd85bc1c7 100644 --- a/app/serializers/merge_request_ai_entity.rb +++ b/app/serializers/merge_request_ai_entity.rb @@ -1,16 +1,16 @@ # frozen_string_literal: true -class MergeRequestAiEntity < Api::Entities::MergeRequestBasic +class MergeRequestAiEntity < ::API::Entities::MergeRequestBasic expose :mr_comments do |_mr, options| options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit]/2) end - expose :diff do |_mr, options| + expose :diff do |mr, options| Gitlab::Llm::Utils::MergeRequestTool.extract_diff( - source_project: options[:resource].source_project, - source_branch: options[:resource].source_branch, - target_project: options[:resource].target_project, - target_branch: options[:resource].target_branch, + source_project: mr.source_project, + source_branch: mr.source_branch, + target_project: mr.target_project, + target_branch: mr.target_branch, character_limit: options[:notes_limit]/2 ) end diff --git a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb index dfb30af0be65b2..0eaf11106cbdd3 100644 --- a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb +++ b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb @@ -8,7 +8,7 @@ module MergeRequestReader class Executor < Identifier include Concerns::ReaderTooling - RESOURCE_NAME = 'merge request' + RESOURCE_NAME = 'mergerequest' NAME = "MergeRequestReader" HUMAN_NAME = 'Merge Request Search' DESCRIPTION = 'Gets the content of the current merge request (also referenced as this or that) the user sees ' \ diff --git a/ee/lib/gitlab/llm/utils/merge_request_tool.rb b/ee/lib/gitlab/llm/utils/merge_request_tool.rb index 9ea47bc39069a8..f03fe3ec2d889a 100644 --- a/ee/lib/gitlab/llm/utils/merge_request_tool.rb +++ b/ee/lib/gitlab/llm/utils/merge_request_tool.rb @@ -24,7 +24,7 @@ def self.extract_diff(source_project:, source_branch:, target_project:, target_b end.join.truncate_words(character_limit) end - def diff_output(old_path, new_path, diff) + def self.diff_output(old_path, new_path, diff) <<~DIFF --- #{old_path} +++ #{new_path} -- GitLab From 31c6cbde34b83a8c792ca662e2dde9d9b412478e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 21 May 2024 10:54:43 +0200 Subject: [PATCH 03/32] Improve real-request specs --- .../completions/chat_real_requests_spec.rb | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb b/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb index b4c08043f2a0bf..72be80aaea2b6f 100644 --- a/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb +++ b/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb @@ -83,12 +83,27 @@ end end - context 'without tool' do + context 'with predefined MR' do + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + where(:input_template, :tools) do + 'Summarize this Merge Request' | %w[MergeRequestReader] + 'Summarize %s Merge Request' | %w[MergeRequestReader] + 'Why did this pipeline fail?' | [] + end + + with_them do + let(:resource) { merge_request } + let(:input) { format(input_template, merge_request_identifier: merge_request.to_reference(full: true).to_s) } + + it_behaves_like 'successful prompt processing' + end + end + + context 'without predefined tools' do let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } where(:input_template, :tools) do - 'Summarize this Merge Request' | [] - 'Summarize %s Merge Request' | [] 'Why did this pipeline fail?' | [] end -- GitLab From 39959ea0980073a6fb1a31b669bee3d6736b9bc7 Mon Sep 17 00:00:00 2001 From: Nathan Weinshenker Date: Thu, 23 May 2024 18:58:30 -0700 Subject: [PATCH 04/32] Added test specs to the following MR for merge request reader --- app/serializers/merge_request_ai_entity.rb | 4 +- .../merge_request_reader/prompts/anthropic.rb | 2 +- .../merge_request_reader/executor_spec.rb | 270 ++++++++++++++++++ .../prompts/anthropic_spec.rb | 37 +++ .../lib/gitlab/llm/completions/chat_spec.rb | 1 + .../llm/utils/merge_request_tool_spec.rb | 64 +++++ .../ai/ai_resource/merge_request_spec.rb | 37 +++ .../schemas/entities/merge_request_ai.json | 11 + .../merge_request_ai_entity_spec.rb | 39 +++ .../merge_request_serializer_spec.rb | 8 + 10 files changed, 470 insertions(+), 3 deletions(-) create mode 100644 ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb create mode 100644 ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb create mode 100644 ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb create mode 100644 ee/spec/models/ai/ai_resource/merge_request_spec.rb create mode 100644 spec/fixtures/api/schemas/entities/merge_request_ai.json create mode 100644 spec/serializers/merge_request_ai_entity_spec.rb diff --git a/app/serializers/merge_request_ai_entity.rb b/app/serializers/merge_request_ai_entity.rb index 110d2fd85bc1c7..ad68a5953251bc 100644 --- a/app/serializers/merge_request_ai_entity.rb +++ b/app/serializers/merge_request_ai_entity.rb @@ -2,7 +2,7 @@ class MergeRequestAiEntity < ::API::Entities::MergeRequestBasic expose :mr_comments do |_mr, options| - options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit]/2) + options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit] / 2) end expose :diff do |mr, options| @@ -11,7 +11,7 @@ class MergeRequestAiEntity < ::API::Entities::MergeRequestBasic source_branch: mr.source_branch, target_project: mr.target_project, target_branch: mr.target_branch, - character_limit: options[:notes_limit]/2 + character_limit: options[:notes_limit] / 2 ) end end diff --git a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb index 5c4e152b3d4466..c1e1002479e3b7 100644 --- a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb +++ b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb @@ -11,7 +11,7 @@ class Anthropic def self.prompt(options) conversation = Utils::Prompt.role_conversation([ - ::Gitlab::Llm::Chain::Tools::IssueReader::Executor::SYSTEM_PROMPT, + Gitlab::Llm::Chain::Tools::MergeRequestReader::Executor::SYSTEM_PROMPT, Utils::Prompt.as_user(options[:input]), Utils::Prompt.as_assistant(options[:suggestions], "```json \{ diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb new file mode 100644 index 00000000000000..2c42fef0522720 --- /dev/null +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb @@ -0,0 +1,270 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Llm::Chain::Tools::MergeRequestReader::Executor, feature_category: :duo_chat do + RSpec.shared_examples 'success response' do + it 'returns success response' do + ai_request = double + allow(ai_request).to receive(:request).and_return(ai_response) + allow(context).to receive(:ai_request).and_return(ai_request) + resource_serialized = Ai::AiResource::MergeRequest.new(resource) + .serialize_for_ai( + user: context.current_user, + content_limit: ::Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic::MAX_CHARACTERS + ).to_xml(root: :root, skip_types: true, skip_instruct: true) + + response = "Please use this information about identified mergerequest: #{resource_serialized}" + + expect(tool.execute.content).to eq(response) + end + end + + RSpec.shared_examples 'merge request not found response' do + it 'returns success response' do + allow(tool).to receive(:request).and_return(ai_response) + + response = "I am sorry, I am unable to find what you are looking for." + expect(tool.execute.content).to eq(response) + end + end + + describe '#name' do + it 'returns tool name' do + expect(described_class::NAME).to eq('MergeRequestReader') + end + + it 'returns tool human name' do + expect(described_class::HUMAN_NAME).to eq('Merge Request Search') + end + end + + describe '#description' do + it 'returns tool description' do + expect(described_class::DESCRIPTION) + .to include('Gets the content of the current merge request (also referenced as this or that)') + end + end + + describe '#execute', :saas do + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:group) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be_with_reload(:project) { create(:project, group: group) } + + before_all do + project.add_guest(user) + end + + before do + stub_const("::Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic::MAX_CHARACTERS", + 999999) + allow(tool).to receive(:provider_prompt_class) + .and_return(::Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic) + end + + context 'when merge request is identified' do + let_it_be(:merge_request1) { create(:merge_request, source_project: project, source_branch: 'branch-1') } + let_it_be(:merge_request2) { create(:merge_request, source_project: project, source_branch: 'branch-2') } + let(:context) do + Gitlab::Llm::Chain::GitlabContext.new( + container: project, + resource: merge_request1, + current_user: user, + ai_request: double + ) + end + + let(:tool) { described_class.new(context: context, options: input_variables, stream_response_handler: nil) } + let(:input_variables) do + { input: "user input", suggestions: "Action: MergeRequestReader\nActionInput: #{merge_request1.iid}" } + end + + context 'when user has permission to read resource' do + before do + stub_application_setting(check_namespace_plan: true) + stub_licensed_features(ai_chat: true) + + allow(project.root_ancestor.namespace_settings).to receive(:experiment_settings_allowed?).and_return(true) + project.root_ancestor.update!(experiment_features_enabled: true) + end + + context 'when ai response has invalid JSON' do + it 'retries the ai call' do + input_variables = { input: "user input", suggestions: "" } + tool = described_class.new(context: context, options: input_variables) + + allow(tool).to receive(:request).and_return("random string") + allow(Gitlab::Json).to receive(:parse).and_raise(JSON::ParserError) + + expect(tool).to receive(:request).exactly(3).times + + response = "I am sorry, I am unable to find what you are looking for." + expect(tool.execute.content).to eq(response) + end + end + + context 'when there is a StandardError' do + it 'returns an error' do + input_variables = { input: "user input", suggestions: "" } + tool = described_class.new(context: context, options: input_variables) + + allow(tool).to receive(:request).and_raise(StandardError) + + expect(tool.execute.content).to eq("Unexpected error") + end + end + + context 'when merge request is the current MR in context' do + let(:identifier) { 'current' } + let(:ai_response) { "current\", \"ResourceIdentifier\": \"#{identifier}\"}" } + let(:resource) { merge_request1 } + + it_behaves_like 'success response' + end + + context 'when merge request is identified by iid' do + let(:identifier) { merge_request2.iid } + let(:ai_response) { "iid\", \"ResourceIdentifier\": #{identifier}}" } + let(:resource) { merge_request2 } + + it_behaves_like 'success response' + end + + context 'when is merge request identified with reference' do + let(:identifier) { merge_request2.to_reference(full: true) } + let(:ai_response) do + "reference\", \"ResourceIdentifier\": \"#{identifier}\"}" + end + + let(:resource) { merge_request2 } + + it_behaves_like 'success response' + end + + context 'when MR mistaken with an issue' do + let_it_be(:issue) { create(:issue, project: project) } + + let(:ai_response) { "current\", \"ResourceIdentifier\": \"current\"}" } + + before do + context.resource = issue + end + + it_behaves_like 'merge request not found response' + end + + context 'when context container is a group' do + before do + context.container = group + end + + let(:identifier) { merge_request2.iid } + let(:ai_response) { "iid\", \"ResourceIdentifier\": #{identifier}}" } + let(:resource) { merge_request2 } + + it_behaves_like 'success response' + + context 'when multiple merge requests are identified' do + let_it_be(:project) { create(:project, group: group) } + let_it_be(:merge_request3) { create(:merge_request, iid: merge_request2.iid, project: project) } + + let(:identifier) { merge_request2.iid } + let(:ai_response) { "iid\", \"ResourceIdentifier\": #{identifier}}" } + + it_behaves_like 'merge request not found response' + end + end + + context 'when context container is a project namespace' do + before do + context.container = project.project_namespace + end + + context 'when merge request is the current merge_request in context' do + let(:identifier) { merge_request2.iid } + let(:ai_response) { "iid\", \"ResourceIdentifier\": #{identifier}}" } + let(:resource) { merge_request2 } + + it_behaves_like 'success response' + end + end + + context 'when context container is nil' do + before do + context.container = nil + end + + context 'when merge request is identified by iid' do + let(:identifier) { merge_request2.iid } + let(:ai_response) { "iid\", \"ResourceIdentifier\": #{identifier}}" } + + it_behaves_like 'merge request not found response' + end + + context 'when merge request is the current MR in context' do + let(:identifier) { 'current' } + let(:ai_response) { "current\", \"ResourceIdentifier\": \"#{identifier}\"}" } + let(:resource) { merge_request1 } + + it_behaves_like 'success response' + end + + context 'when is merge request identified with reference' do + let(:identifier) { merge_request2.to_reference(full: true) } + let(:ai_response) do + "reference\", \"ResourceIdentifier\": \"#{identifier}\"}" + end + + let(:resource) { merge_request2 } + + it_behaves_like 'success response' + end + + context 'when is merge request identified with not-full reference' do + let(:identifier) { merge_request2.to_reference(full: false) } + let(:ai_response) do + "reference\", \"ResourceIdentifier\": \"#{identifier}\"}" + end + + it_behaves_like 'merge request not found response' + end + + context 'when group does not have ai enabled' do + let(:identifier) { 'current' } + let(:ai_response) { "current\", \"ResourceIdentifier\": \"#{identifier}\"}" } + + before do + stub_licensed_features(ai_chat: false) + end + + it 'returns success response' do + allow(tool).to receive(:request).and_return(ai_response) + + response = 'This feature is only allowed in groups or projects that enable this feature.' + + expect(tool.execute.content).to eq(response) + end + end + end + + context 'when merge request was already identified' do + let(:resource_iid) { merge_request1.iid } + let(:ai_response) { "iid\", \"ResourceIdentifier\": #{merge_request1.iid}}" } + + before do + context.tools_used << described_class + end + + it 'returns already identified response' do + ai_request = double + allow(ai_request).to receive_message_chain(:complete, :dig, :to_s, :strip).and_return(ai_response) + allow(context).to receive(:ai_request).and_return(ai_request) + + response = "You already have identified the mergerequest #{context.resource.to_global_id}, read carefully." + expect(tool.execute.content).to eq(response) + end + end + end + end + end +end diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb new file mode 100644 index 00000000000000..75eff3fc43b08a --- /dev/null +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic, feature_category: :duo_chat do + describe '.prompt' do + let(:options) { { input: 'test input', suggestions: 'test suggestions' } } + + describe '.prompt' do + it 'returns prompt' do + prompt = described_class.prompt(options)[:prompt] + + expect(prompt.length).to eq(3) + + binding.pry + expect(prompt[0][:role]).to eq(:system) + expect(prompt[0][:content]).to eq(system_prompt) + + expect(prompt[1][:role]).to eq(:user) + expect(prompt[1][:content]).to eq(options[:input]) + + expect(prompt[2][:role]).to eq(:assistant) + expect(prompt[2][:content]).to include(options[:suggestions], "\"ResourceIdentifierType\": \"") + end + + it "calls with haiku model" do + model = described_class.prompt(options)[:options][:model] + + expect(model).to eq(::Gitlab::Llm::AiGateway::Client::CLAUDE_3_HAIKU) + end + end + + def system_prompt + Gitlab::Llm::Chain::Tools::MergeRequestReader::Executor::SYSTEM_PROMPT[1] + end + end +end diff --git a/ee/spec/lib/gitlab/llm/completions/chat_spec.rb b/ee/spec/lib/gitlab/llm/completions/chat_spec.rb index e54e76b3f90c4b..931c56ce2e4002 100644 --- a/ee/spec/lib/gitlab/llm/completions/chat_spec.rb +++ b/ee/spec/lib/gitlab/llm/completions/chat_spec.rb @@ -62,6 +62,7 @@ let(:tools) do [ ::Gitlab::Llm::Chain::Tools::IssueReader, + ::Gitlab::Llm::Chain::Tools::MergeRequestReader, ::Gitlab::Llm::Chain::Tools::GitlabDocumentation, ::Gitlab::Llm::Chain::Tools::EpicReader, ::Gitlab::Llm::Chain::Tools::CiEditorAssistant diff --git a/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb b/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb new file mode 100644 index 00000000000000..0c5821d56885d6 --- /dev/null +++ b/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Llm::Utils::MergeRequestTool, feature_category: :ai_abstraction_layer do + describe '.extract_diff' do + let(:source_project) { create(:project) } + let(:source_branch) { 'main' } + let(:target_project) { create(:project) } + let(:target_branch) { 'feature' } + let(:character_limit) { 1000 } + + subject do + described_class.extract_diff(source_project: source_project, source_branch: source_branch, + target_project: target_project, target_branch: target_branch, character_limit: character_limit) + end + + context 'when compare service returns a result' do + let(:compare) { double(:compare, raw_diffs: raw_diffs) } + let(:raw_diffs) { [raw_diff_1, raw_diff_2] } + let(:raw_diff_1) do + double(:raw_diff, diff: "diff --git a/file1.rb b/file1.rb\n+foo\n-bar", old_path: 'file1.rb', new_path: 'file1.rb', + has_binary_notice?: false) + end + + let(:raw_diff_2) do + double(:raw_diff, diff: "diff --git a/file2.rb b/file2.rb\n+baz\n-qux", old_path: 'file2.rb', new_path: 'file2.rb', + has_binary_notice?: false) + end + + before do + allow(CompareService).to receive(:new).and_return(double(:compare_service, execute: compare)) + end + + it 'returns the formatted diff' do + expected_diff = "--- file1.rb\n+++ file1.rb\n+foo\n-bar\n\n--- file2.rb\n+++ file2.rb\n+baz\n-qux" + expect(subject).to eq(expected_diff) + end + end + + context 'when compare service returns nil' do + before do + allow(CompareService).to receive(:new).and_return(double(:compare_service, execute: nil)) + end + + it 'returns nil' do + expect(subject).to be_nil + end + end + end + + describe '.diff_output' do + let(:old_path) { 'file.rb' } + let(:new_path) { 'file.rb' } + let(:diff) { "+foo\n-bar" } + + subject { described_class.diff_output(old_path, new_path, diff) } + + it 'returns the formatted diff' do + expected_diff = "--- file.rb\n+++ file.rb\n+foo\n-bar" + expect(subject).to eq(expected_diff) + end + end +end diff --git a/ee/spec/models/ai/ai_resource/merge_request_spec.rb b/ee/spec/models/ai/ai_resource/merge_request_spec.rb new file mode 100644 index 00000000000000..0442312b4ed945 --- /dev/null +++ b/ee/spec/models/ai/ai_resource/merge_request_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ai::AiResource::MergeRequest, feature_category: :duo_chat do + let(:merge_request) { build(:merge_request) } + let(:user) { build(:user) } + + subject(:wrapped_merge_request) { described_class.new(merge_request) } + + describe '#serialize_for_ai' do + it 'calls the serializations class' do + expect(MergeRequestSerializer).to receive_message_chain(:new, :represent) + .with(current_user: user) + .with(merge_request, { + user: user, + notes_limit: 100, + serializer: 'ai', + resource: wrapped_merge_request + }) + + wrapped_merge_request.serialize_for_ai(user: user, content_limit: 100) + end + end + + describe '#current_page_sentence' do + it 'returns prompt' do + expect(wrapped_merge_request.current_page_sentence).to include(" utilize it instead of using the 'MergeRequestReader' tool.") + end + end + + describe '#current_page_short_description' do + it 'returns prompt' do + expect(wrapped_merge_request.current_page_short_description).to include("The title of the merge request is '#{merge_request.title}'.") + end + end +end diff --git a/spec/fixtures/api/schemas/entities/merge_request_ai.json b/spec/fixtures/api/schemas/entities/merge_request_ai.json new file mode 100644 index 00000000000000..a56fa2075bc141 --- /dev/null +++ b/spec/fixtures/api/schemas/entities/merge_request_ai.json @@ -0,0 +1,11 @@ +{ + "type": "object", + "properties": { + "mr_comments": { + "type": "array" + }, + "diff": { + "type": "string" + } + } +} diff --git a/spec/serializers/merge_request_ai_entity_spec.rb b/spec/serializers/merge_request_ai_entity_spec.rb new file mode 100644 index 00000000000000..66709be4602efc --- /dev/null +++ b/spec/serializers/merge_request_ai_entity_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequestAiEntity, feature_category: :api do + let_it_be(:user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request) } + let(:notes_limit) {1000} + + let(:entity) { described_class.new(merge_request, current_user: user, resouce: merge_request, notes_limit: notes_limit) } + + subject(:basic_entity) { entity.as_json } + + it "exposes basic entity fields" do + expected_fields = %i[merged_by merge_user merged_at closed_by closed_at title_html description_html target_branch + source_branch user_notes_count upvotes downvotes author assignees assignee reviewers source_project_id target_project + id labels draft imported imported_from work_in_progress milestone merge_when_pipeline_succeeds merge_status detailed_ + merge_status sha merge_commit_sha squash_commit_sha discussion_locked should_remove_source_branch force_remove_source_ + branch prepared_at allow_collaboration allow_maintainer_to_push reference references web_url time_stats squash squash_ + on_merge task_completion_status has_conflicts blocking_discussions_resolved] + + is_expected.to include(*expected_fields) + end + + context "with mr comments on the entity" do + let!(:note) { create(:note_on_merge_request, noteable: merge_request, project: merge_request.project) } + let!(:note2) { create(:note_on_merge_request, noteable: merge_request, project: merge_request.project) } + + it "exposes the number of comments" do + expect(subject[:mr_comments]).to eq([notes, note2].join("\n")) + end + end + + context "with diff on the entity" do + it "exposes the diff information" do + expect(subject[:diff]).to include("---something") + end + end +end diff --git a/spec/serializers/merge_request_serializer_spec.rb b/spec/serializers/merge_request_serializer_spec.rb index 6baf5cae06a50e..a8e67f33c2e816 100644 --- a/spec/serializers/merge_request_serializer_spec.rb +++ b/spec/serializers/merge_request_serializer_spec.rb @@ -85,6 +85,14 @@ end end + context 'poll merge request ai serialization' do + let(:serializer) { 'merge_request_ai' } + + it 'matches merge_request_ai json schema' do + expect(json_entity).to match_schema('entities/merge_request_ai') + end + end + context 'no serializer' do let(:serializer) { nil } -- GitLab From 318f94a0b1d8e4db0874015a62c3c17e85c1f50d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 10:36:41 +0200 Subject: [PATCH 05/32] Fix failing merge request reader specs --- .../llm/chain/tools/merge_request_reader/executor_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb index 2c42fef0522720..80b26cbaf845c6 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb @@ -52,7 +52,7 @@ let_it_be_with_reload(:project) { create(:project, group: group) } before_all do - project.add_guest(user) + project.add_developer(user) end before do @@ -166,7 +166,7 @@ context 'when multiple merge requests are identified' do let_it_be(:project) { create(:project, group: group) } - let_it_be(:merge_request3) { create(:merge_request, iid: merge_request2.iid, project: project) } + let_it_be(:merge_request3) { create(:merge_request, iid: merge_request2.iid, source_project: project) } let(:identifier) { merge_request2.iid } let(:ai_response) { "iid\", \"ResourceIdentifier\": #{identifier}}" } -- GitLab From 541c6922a5745ee12304f5b43cb4adb9b6d6dec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 11:35:32 +0200 Subject: [PATCH 06/32] Fix entity specs --- .../prompts/anthropic_spec.rb | 47 +++++++++---------- .../merge_request_ai_entity_spec.rb | 43 ++++++++++------- 2 files changed, 50 insertions(+), 40 deletions(-) diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb index 75eff3fc43b08a..709c08893af64a 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb @@ -7,31 +7,30 @@ let(:options) { { input: 'test input', suggestions: 'test suggestions' } } describe '.prompt' do - it 'returns prompt' do - prompt = described_class.prompt(options)[:prompt] - - expect(prompt.length).to eq(3) - - binding.pry - expect(prompt[0][:role]).to eq(:system) - expect(prompt[0][:content]).to eq(system_prompt) - - expect(prompt[1][:role]).to eq(:user) - expect(prompt[1][:content]).to eq(options[:input]) - - expect(prompt[2][:role]).to eq(:assistant) - expect(prompt[2][:content]).to include(options[:suggestions], "\"ResourceIdentifierType\": \"") - end - - it "calls with haiku model" do - model = described_class.prompt(options)[:options][:model] - - expect(model).to eq(::Gitlab::Llm::AiGateway::Client::CLAUDE_3_HAIKU) - end + it 'returns prompt' do + prompt = described_class.prompt(options)[:prompt] + + expect(prompt.length).to eq(3) + + expect(prompt[0][:role]).to eq(:system) + expect(prompt[0][:content]).to eq(system_prompt) + + expect(prompt[1][:role]).to eq(:user) + expect(prompt[1][:content]).to eq(options[:input]) + + expect(prompt[2][:role]).to eq(:assistant) + expect(prompt[2][:content]).to include(options[:suggestions], "\"ResourceIdentifierType\": \"") end - - def system_prompt - Gitlab::Llm::Chain::Tools::MergeRequestReader::Executor::SYSTEM_PROMPT[1] + + it "calls with haiku model" do + model = described_class.prompt(options)[:options][:model] + + expect(model).to eq(::Gitlab::Llm::AiGateway::Client::CLAUDE_3_HAIKU) end + end + + def system_prompt + Gitlab::Llm::Chain::Tools::MergeRequestReader::Executor::SYSTEM_PROMPT[1] + end end end diff --git a/spec/serializers/merge_request_ai_entity_spec.rb b/spec/serializers/merge_request_ai_entity_spec.rb index 66709be4602efc..afba6a51858447 100644 --- a/spec/serializers/merge_request_ai_entity_spec.rb +++ b/spec/serializers/merge_request_ai_entity_spec.rb @@ -2,38 +2,49 @@ require 'spec_helper' -RSpec.describe MergeRequestAiEntity, feature_category: :api do - let_it_be(:user) { create(:user) } - let_it_be(:merge_request) { create(:merge_request) } - let(:notes_limit) {1000} - - let(:entity) { described_class.new(merge_request, current_user: user, resouce: merge_request, notes_limit: notes_limit) } +RSpec.describe MergeRequestAiEntity, feature_category: :ai_abstraction_layer do + let_it_be(:user) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it + let_it_be(:merge_request) { create(:merge_request) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it + let(:notes_limit) { 1000 } + + let(:entity) do + described_class.new(merge_request, + user: user, + resource: Ai::AiResource::MergeRequest.new(merge_request), + notes_limit: notes_limit) + end subject(:basic_entity) { entity.as_json } + before do + merge_request.project.add_developer(user) + end + it "exposes basic entity fields" do - expected_fields = %i[merged_by merge_user merged_at closed_by closed_at title_html description_html target_branch - source_branch user_notes_count upvotes downvotes author assignees assignee reviewers source_project_id target_project - id labels draft imported imported_from work_in_progress milestone merge_when_pipeline_succeeds merge_status detailed_ - merge_status sha merge_commit_sha squash_commit_sha discussion_locked should_remove_source_branch force_remove_source_ - branch prepared_at allow_collaboration allow_maintainer_to_push reference references web_url time_stats squash squash_ - on_merge task_completion_status has_conflicts blocking_discussions_resolved] + expected_fields = %i[ + merged_by merge_user merged_at closed_by closed_at target_branch user_notes_count upvotes downvotes + author assignees assignee reviewers source_project_id target_project_id labels draft work_in_progress + milestone merge_when_pipeline_succeeds merge_status detailed_merge_status sha merge_commit_sha + squash_commit_sha discussion_locked should_remove_source_branch force_remove_source_branch prepared_at + reference references web_url time_stats squash task_completion_status has_conflicts blocking_discussions_resolved + imported imported_from + ] is_expected.to include(*expected_fields) end context "with mr comments on the entity" do - let!(:note) { create(:note_on_merge_request, noteable: merge_request, project: merge_request.project) } - let!(:note2) { create(:note_on_merge_request, noteable: merge_request, project: merge_request.project) } + let!(:note) { create(:note_on_merge_request, noteable: merge_request, project: merge_request.project) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it + let!(:note2) { create(:note_on_merge_request, noteable: merge_request, project: merge_request.project) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it it "exposes the number of comments" do - expect(subject[:mr_comments]).to eq([notes, note2].join("\n")) + expect(basic_entity[:mr_comments]).to match_array([note.note, note2.note]) end end context "with diff on the entity" do it "exposes the diff information" do - expect(subject[:diff]).to include("---something") + expect(basic_entity[:diff]).to include("--- CHANGELOG") end end end -- GitLab From 407da812c97b54457c3b46b2f29b4e9762e0b27b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 11:47:54 +0200 Subject: [PATCH 07/32] Add feature flag guard --- .../ai_merge_request_reader_for_chat.yml | 9 +++++++++ ee/lib/gitlab/llm/completions/chat.rb | 9 +++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 ee/config/feature_flags/gitlab_com_derisk/ai_merge_request_reader_for_chat.yml diff --git a/ee/config/feature_flags/gitlab_com_derisk/ai_merge_request_reader_for_chat.yml b/ee/config/feature_flags/gitlab_com_derisk/ai_merge_request_reader_for_chat.yml new file mode 100644 index 00000000000000..cdb18f9896bd1d --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/ai_merge_request_reader_for_chat.yml @@ -0,0 +1,9 @@ +--- +name: ai_merge_request_reader_for_chat +feature_issue_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/153616 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/463499 +milestone: '17.1' +group: group::duo chat +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/lib/gitlab/llm/completions/chat.rb b/ee/lib/gitlab/llm/completions/chat.rb index e2829ee65dae07..5d070a2376a26e 100644 --- a/ee/lib/gitlab/llm/completions/chat.rb +++ b/ee/lib/gitlab/llm/completions/chat.rb @@ -11,7 +11,6 @@ class Chat < Base TOOLS = [ ::Gitlab::Llm::Chain::Tools::IssueReader, - ::Gitlab::Llm::Chain::Tools::MergeRequestReader, ::Gitlab::Llm::Chain::Tools::GitlabDocumentation, ::Gitlab::Llm::Chain::Tools::EpicReader, ::Gitlab::Llm::Chain::Tools::CiEditorAssistant @@ -84,7 +83,13 @@ def execute # allows conditional logic e.g. feature flagging def tools - TOOLS + tools = TOOLS + + if Feature.enabled?(:ai_merge_request_reader_for_chat, user) + tools << ::Gitlab::Llm::Chain::Tools::MergeRequestReader + end + + tools end def response_post_processing -- GitLab From c8becdfbafc6a1ddf3d50e9852a38d0e58bb6b5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 12:06:37 +0200 Subject: [PATCH 08/32] Add specs for feature flag --- ee/lib/gitlab/llm/completions/chat.rb | 2 +- .../lib/gitlab/llm/completions/chat_spec.rb | 22 +++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/ee/lib/gitlab/llm/completions/chat.rb b/ee/lib/gitlab/llm/completions/chat.rb index 5d070a2376a26e..f7549d8212b505 100644 --- a/ee/lib/gitlab/llm/completions/chat.rb +++ b/ee/lib/gitlab/llm/completions/chat.rb @@ -83,7 +83,7 @@ def execute # allows conditional logic e.g. feature flagging def tools - tools = TOOLS + tools = TOOLS.dup if Feature.enabled?(:ai_merge_request_reader_for_chat, user) tools << ::Gitlab::Llm::Chain::Tools::MergeRequestReader diff --git a/ee/spec/lib/gitlab/llm/completions/chat_spec.rb b/ee/spec/lib/gitlab/llm/completions/chat_spec.rb index 931c56ce2e4002..ace4821c844c33 100644 --- a/ee/spec/lib/gitlab/llm/completions/chat_spec.rb +++ b/ee/spec/lib/gitlab/llm/completions/chat_spec.rb @@ -62,7 +62,6 @@ let(:tools) do [ ::Gitlab::Llm::Chain::Tools::IssueReader, - ::Gitlab::Llm::Chain::Tools::MergeRequestReader, ::Gitlab::Llm::Chain::Tools::GitlabDocumentation, ::Gitlab::Llm::Chain::Tools::EpicReader, ::Gitlab::Llm::Chain::Tools::CiEditorAssistant @@ -188,6 +187,7 @@ allow(Gitlab::Llm::Chain::Requests::AiGateway).to receive(:new).and_return(ai_request) allow(context).to receive(:tools_used).and_return([Gitlab::Llm::Chain::Tools::IssueReader::Executor]) stub_saas_features(duo_chat_categorize_question: true) + stub_feature_flags(ai_merge_request_reader_for_chat: false) end context 'when resource is an issue' do @@ -228,7 +228,7 @@ expect(::Gitlab::Llm::Chain::GitlabContext).to receive(:new) .with(current_user: user, container: expected_container, resource: resource, ai_request: ai_request, extra_resource: extra_resource, request_id: 'uuid', - current_file: current_file) + current_file: current_file, agent_version: agent_version) .and_return(context) expect(categorize_service).to receive(:execute) expect(Llm::ExecuteMethodService).to receive(:new) @@ -239,6 +239,24 @@ end end + context 'with merge request reader allowed' do + before do + stub_feature_flags(ai_merge_request_reader_for_chat: true) + end + + let(:tools) do + [ + ::Gitlab::Llm::Chain::Tools::IssueReader, + ::Gitlab::Llm::Chain::Tools::GitlabDocumentation, + ::Gitlab::Llm::Chain::Tools::EpicReader, + ::Gitlab::Llm::Chain::Tools::CiEditorAssistant, + ::Gitlab::Llm::Chain::Tools::MergeRequestReader + ] + end + + it_behaves_like 'tool behind a feature flag' + end + context 'when message is a slash command' do shared_examples_for 'slash command execution' do let(:executor) { instance_double(Gitlab::Llm::Chain::Tools::ExplainCode::Executor) } -- GitLab From 565ec81b61736592de494968f654dc8221720466 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 15:09:42 +0200 Subject: [PATCH 09/32] Fix resource name problem --- .../llm/chain/tools/merge_request_reader/executor.rb | 10 +++++----- ee/lib/gitlab/llm/chain/tools/tool.rb | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb index 0eaf11106cbdd3..7cd7973f1015f2 100644 --- a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb +++ b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb @@ -8,13 +8,13 @@ module MergeRequestReader class Executor < Identifier include Concerns::ReaderTooling - RESOURCE_NAME = 'mergerequest' + RESOURCE_NAME = 'merge request' NAME = "MergeRequestReader" HUMAN_NAME = 'Merge Request Search' - DESCRIPTION = 'Gets the content of the current merge request (also referenced as this or that) the user sees ' \ - 'or a specific merge request identified by an ID or a URL.' \ - 'In this context, "merge request" means part of work that is ready to be merged. ' \ - 'Action Input for this tool should be the original question or merge request identifier.' + DESCRIPTION = 'Gets the content of the current merge request (also referenced as this or that, or MR)' \ + 'the user sees or a specific merge request identified by an ID or a URL.' \ + 'In this context, "merge request" means part of work that is ready to be merged. ' \ + 'Action Input for this tool should be the original question or merge request identifier.' EXAMPLE = <<~PROMPT diff --git a/ee/lib/gitlab/llm/chain/tools/tool.rb b/ee/lib/gitlab/llm/chain/tools/tool.rb index 4e23d40ddd4f4c..785b0aa2da6c6a 100644 --- a/ee/lib/gitlab/llm/chain/tools/tool.rb +++ b/ee/lib/gitlab/llm/chain/tools/tool.rb @@ -54,7 +54,8 @@ def perform end def current_resource?(resource_identifier_type, resource_name) - resource_identifier_type == 'current' && context.resource.class.name.downcase == resource_name + resource_identifier_type == 'current' && + context.resource.class.name.underscore == resource_name.tr(' ', '_') end def projects_from_context -- GitLab From 4333fe689f64fee209a76677df5a3ed1cc50ab2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 17:46:49 +0200 Subject: [PATCH 10/32] Fix specs, add additional action --- .../ee/projects/merge_requests/application_controller.rb | 8 ++++++++ .../llm/chain/tools/merge_request_reader/executor.rb | 2 +- .../llm/chain/tools/merge_request_reader/executor_spec.rb | 6 +++--- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb index 8dbc3398caaed2..679ca3999afd4c 100644 --- a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb @@ -6,6 +6,10 @@ module MergeRequests module ApplicationController extend ActiveSupport::Concern + prepended do + before_action :set_application_context! + end + private def merge_request_includes(association) @@ -74,6 +78,10 @@ def clamp_approvals_before_merge(mr_params) mr_params end + + def set_application_context! + ::Gitlab::ApplicationContext.push(ai_resource: merge_request.try(:to_global_id)) + end end end end diff --git a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb index 7cd7973f1015f2..b0b93306e1c8dc 100644 --- a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb +++ b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/executor.rb @@ -11,7 +11,7 @@ class Executor < Identifier RESOURCE_NAME = 'merge request' NAME = "MergeRequestReader" HUMAN_NAME = 'Merge Request Search' - DESCRIPTION = 'Gets the content of the current merge request (also referenced as this or that, or MR)' \ + DESCRIPTION = 'Gets the content of the current merge request (also referenced as this or that, or MR) ' \ 'the user sees or a specific merge request identified by an ID or a URL.' \ 'In this context, "merge request" means part of work that is ready to be merged. ' \ 'Action Input for this tool should be the original question or merge request identifier.' diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb index 80b26cbaf845c6..09c467defff889 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb @@ -14,7 +14,7 @@ content_limit: ::Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic::MAX_CHARACTERS ).to_xml(root: :root, skip_types: true, skip_instruct: true) - response = "Please use this information about identified mergerequest: #{resource_serialized}" + response = "Please use this information about identified merge request: #{resource_serialized}" expect(tool.execute.content).to eq(response) end @@ -42,7 +42,7 @@ describe '#description' do it 'returns tool description' do expect(described_class::DESCRIPTION) - .to include('Gets the content of the current merge request (also referenced as this or that)') + .to include('Gets the content of the current merge request (also referenced as this or that, or MR)') end end @@ -260,7 +260,7 @@ allow(ai_request).to receive_message_chain(:complete, :dig, :to_s, :strip).and_return(ai_response) allow(context).to receive(:ai_request).and_return(ai_request) - response = "You already have identified the mergerequest #{context.resource.to_global_id}, read carefully." + response = "You already have identified the merge request #{context.resource.to_global_id}, read carefully." expect(tool.execute.content).to eq(response) end end -- GitLab From bb5c718568dbb1f58044a06aa5cd343a7f53ceff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 18:34:10 +0200 Subject: [PATCH 11/32] Add proper assigning of resource id --- app/controllers/projects/merge_requests_controller.rb | 1 + .../ee/projects/merge_requests/application_controller.rb | 8 -------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2bd8d180035d76..ab5887a6966e01 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -32,6 +32,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo before_action :check_search_rate_limit!, only: [:index], if: -> { params[:search].present? } before_action :authenticate_user!, only: [:assign_related_issues] before_action :check_user_can_push_to_source_branch!, only: [:rebase] + before_action :set_application_context!, only: [:show, :diffs, :commits, :pipelines] before_action only: :index do push_frontend_feature_flag(:mr_approved_filter, type: :ops) diff --git a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb index 679ca3999afd4c..8dbc3398caaed2 100644 --- a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb @@ -6,10 +6,6 @@ module MergeRequests module ApplicationController extend ActiveSupport::Concern - prepended do - before_action :set_application_context! - end - private def merge_request_includes(association) @@ -78,10 +74,6 @@ def clamp_approvals_before_merge(mr_params) mr_params end - - def set_application_context! - ::Gitlab::ApplicationContext.push(ai_resource: merge_request.try(:to_global_id)) - end end end end -- GitLab From 10acf2a77669db4961d06351c7a24d2581a255cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 24 May 2024 18:35:07 +0200 Subject: [PATCH 12/32] Add proper assigning of resource id - move to ee --- app/controllers/projects/merge_requests_controller.rb | 1 - ee/app/controllers/ee/projects/merge_requests_controller.rb | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index ab5887a6966e01..2bd8d180035d76 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -32,7 +32,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo before_action :check_search_rate_limit!, only: [:index], if: -> { params[:search].present? } before_action :authenticate_user!, only: [:assign_related_issues] before_action :check_user_can_push_to_source_branch!, only: [:rebase] - before_action :set_application_context!, only: [:show, :diffs, :commits, :pipelines] before_action only: :index do push_frontend_feature_flag(:mr_approved_filter, type: :ops) diff --git a/ee/app/controllers/ee/projects/merge_requests_controller.rb b/ee/app/controllers/ee/projects/merge_requests_controller.rb index 0c38d46705faa6..2d13b3e1bf14a3 100644 --- a/ee/app/controllers/ee/projects/merge_requests_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests_controller.rb @@ -30,6 +30,7 @@ module MergeRequestsController before_action :authorize_read_licenses!, only: [:license_scanning_reports, :license_scanning_reports_collapsed] before_action :authorize_read_security_reports!, only: [:security_reports] + before_action :set_application_context!, only: [:show, :diffs, :commits, :pipelines] feature_category :vulnerability_management, [:container_scanning_reports, :dependency_scanning_reports, :sast_reports, :secret_detection_reports, :dast_reports, -- GitLab From 5307a27929ebbabe054df13ab1141270e9056e85 Mon Sep 17 00:00:00 2001 From: Nathan Weinshenker Date: Fri, 24 May 2024 12:13:13 -0700 Subject: [PATCH 13/32] Fix rubocop errors --- app/serializers/merge_request_ai_entity.rb | 2 +- ee/app/models/ai/ai_resource/merge_request.rb | 2 +- .../llm/utils/merge_request_tool_spec.rb | 28 +++++++++---------- .../ai/ai_resource/merge_request_spec.rb | 6 ++-- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/app/serializers/merge_request_ai_entity.rb b/app/serializers/merge_request_ai_entity.rb index ad68a5953251bc..f62e1be21db5bd 100644 --- a/app/serializers/merge_request_ai_entity.rb +++ b/app/serializers/merge_request_ai_entity.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class MergeRequestAiEntity < ::API::Entities::MergeRequestBasic +class MergeRequestAiEntity < ::API::Entities::MergeRequestBasic # rubocop:disable Gitlab/NamespacedClass -- serializer doesn't need module expose :mr_comments do |_mr, options| options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit] / 2) end diff --git a/ee/app/models/ai/ai_resource/merge_request.rb b/ee/app/models/ai/ai_resource/merge_request.rb index 081a5b25b4858f..dc6b7f52b08911 100644 --- a/ee/app/models/ai/ai_resource/merge_request.rb +++ b/ee/app/models/ai/ai_resource/merge_request.rb @@ -6,7 +6,7 @@ class MergeRequest < Ai::AiResource::BaseAiResource include Ai::AiResource::Concerns::Noteable def serialize_for_ai(user:, content_limit:) - ::MergeRequestSerializer.new(current_user: user) # rubocop: disable CodeReuse/Serializer + ::MergeRequestSerializer.new(current_user: user) # rubocop: disable CodeReuse/Serializer -- existing serializer .represent(resource, { user: user, notes_limit: content_limit, diff --git a/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb b/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb index 0c5821d56885d6..3bcee7ed061d55 100644 --- a/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb +++ b/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb @@ -4,47 +4,47 @@ RSpec.describe Gitlab::Llm::Utils::MergeRequestTool, feature_category: :ai_abstraction_layer do describe '.extract_diff' do - let(:source_project) { create(:project) } + let_it_be(:source_project) { create(:project) } let(:source_branch) { 'main' } - let(:target_project) { create(:project) } + let_it_be(:target_project) { create(:project) } let(:target_branch) { 'feature' } let(:character_limit) { 1000 } - subject do + subject(:merge_tool_result) do described_class.extract_diff(source_project: source_project, source_branch: source_branch, target_project: target_project, target_branch: target_branch, character_limit: character_limit) end context 'when compare service returns a result' do - let(:compare) { double(:compare, raw_diffs: raw_diffs) } + let(:compare) { instance_double(:compare, raw_diffs: raw_diffs) } let(:raw_diffs) { [raw_diff_1, raw_diff_2] } let(:raw_diff_1) do - double(:raw_diff, diff: "diff --git a/file1.rb b/file1.rb\n+foo\n-bar", old_path: 'file1.rb', new_path: 'file1.rb', - has_binary_notice?: false) + instance_double(:raw_diff, diff: "diff --git a/file1.rb b/file1.rb\n+foo\n-bar", old_path: 'file1.rb', + new_path: 'file1.rb', has_binary_notice?: false) end let(:raw_diff_2) do - double(:raw_diff, diff: "diff --git a/file2.rb b/file2.rb\n+baz\n-qux", old_path: 'file2.rb', new_path: 'file2.rb', - has_binary_notice?: false) + instance_double(:raw_diff, diff: "diff --git a/file2.rb b/file2.rb\n+baz\n-qux", old_path: 'file2.rb', + new_path: 'file2.rb', has_binary_notice?: false) end before do - allow(CompareService).to receive(:new).and_return(double(:compare_service, execute: compare)) + allow(CompareService).to receive(:new).and_return(instance_double(:compare_service, execute: compare)) end it 'returns the formatted diff' do expected_diff = "--- file1.rb\n+++ file1.rb\n+foo\n-bar\n\n--- file2.rb\n+++ file2.rb\n+baz\n-qux" - expect(subject).to eq(expected_diff) + expect(merge_tool_result).to eq(expected_diff) end end context 'when compare service returns nil' do before do - allow(CompareService).to receive(:new).and_return(double(:compare_service, execute: nil)) + allow(CompareService).to receive(:new).and_return(instance_double(:compare_service, execute: nil)) end it 'returns nil' do - expect(subject).to be_nil + expect(merge_tool_result).to be_nil end end end @@ -54,11 +54,11 @@ let(:new_path) { 'file.rb' } let(:diff) { "+foo\n-bar" } - subject { described_class.diff_output(old_path, new_path, diff) } + subject(:merge_request_diff) { described_class.diff_output(old_path, new_path, diff) } it 'returns the formatted diff' do expected_diff = "--- file.rb\n+++ file.rb\n+foo\n-bar" - expect(subject).to eq(expected_diff) + expect(merge_request_diff).to eq(expected_diff) end end end diff --git a/ee/spec/models/ai/ai_resource/merge_request_spec.rb b/ee/spec/models/ai/ai_resource/merge_request_spec.rb index 0442312b4ed945..a33abe15d336a6 100644 --- a/ee/spec/models/ai/ai_resource/merge_request_spec.rb +++ b/ee/spec/models/ai/ai_resource/merge_request_spec.rb @@ -25,13 +25,15 @@ describe '#current_page_sentence' do it 'returns prompt' do - expect(wrapped_merge_request.current_page_sentence).to include(" utilize it instead of using the 'MergeRequestReader' tool.") + expect(wrapped_merge_request.current_page_sentence).to + include(" utilize it instead of using the 'MergeRequestReader' tool.") end end describe '#current_page_short_description' do it 'returns prompt' do - expect(wrapped_merge_request.current_page_short_description).to include("The title of the merge request is '#{merge_request.title}'.") + expect(wrapped_merge_request.current_page_short_description).to + include("The title of the merge request is '#{merge_request.title}'.") end end end -- GitLab From f537214ce8dac8f25773e19bc4f838910673fd21 Mon Sep 17 00:00:00 2001 From: Nathan Weinshenker Date: Fri, 24 May 2024 13:35:24 -0700 Subject: [PATCH 14/32] Resolve test failure and migrate some test over to different class --- .../summarize_new_merge_request_spec.rb | 70 +++++--------- .../llm/utils/merge_request_tool_spec.rb | 94 +++++++++++-------- .../ai/ai_resource/merge_request_spec.rb | 8 +- 3 files changed, 82 insertions(+), 90 deletions(-) diff --git a/ee/spec/lib/gitlab/llm/templates/summarize_new_merge_request_spec.rb b/ee/spec/lib/gitlab/llm/templates/summarize_new_merge_request_spec.rb index 28dc3ef056e9a1..7aaebd2adedd40 100644 --- a/ee/spec/lib/gitlab/llm/templates/summarize_new_merge_request_spec.rb +++ b/ee/spec/lib/gitlab/llm/templates/summarize_new_merge_request_spec.rb @@ -51,54 +51,34 @@ it_behaves_like "prompt without errors" end + end - context "when there is a diff with an edge case" do - let(:good_diff) { { diff: "@@ -0,0 +1 @@hellothere\n+🌚\n" } } - let(:compare) { instance_double(Compare) } - - before do - allow(CompareService).to receive_message_chain(:new, :execute).and_return(compare) - end - - context 'when a diff is not encoded with UTF-8' do - let(:other_diff) do - { diff: "@@ -1 +1 @@\n-This should not be in the prompt\n+#{(0..255).map(&:chr).join}\n" } - end - - let(:diff_files) { Gitlab::Git::DiffCollection.new([good_diff, other_diff]) } - - it 'does not raise any error and not contain the non-UTF diff' do - allow(compare).to receive(:raw_diffs).and_return(diff_files) - - expect { template.to_prompt }.not_to raise_error - - expect(template.to_prompt).to include("hellothere") - expect(template.to_prompt).not_to include("This should not be in the prompt") - end - end - - context 'when a diff contains the binary notice' do - let(:binary_message) { Gitlab::Git::Diff.binary_message('a', 'b') } - let(:other_diff) { { diff: binary_message } } - let(:diff_files) { Gitlab::Git::DiffCollection.new([good_diff, other_diff]) } - - it 'does not contain the binary diff' do - allow(compare).to receive(:raw_diffs).and_return(diff_files) - - expect(template.to_prompt).to include("hellothere") - expect(template.to_prompt).not_to include(binary_message) - end - end + describe '#extracted_diff' do + let(:params) do + { + source_project_id: source_project.id, + source_project: source_project, + target_project: source_project, + source_branch: source_branch, + target_branch: target_branch, + character_limit: described_class::CHARACTER_LIMIT + } + end - context 'when extracted diff is blank' do - before do - allow(template).to receive(:extracted_diff).and_return([]) - end + subject(:template) { described_class.new(user, project, params) } - it 'returns nil' do - expect(template.to_prompt).to be_nil - end - end + it 'returns a diff from merge request tool' do + expect(Gitlab::Llm::Utils::MergeRequestTool).to receive(:extract_diff) + .with( + source_project: params[:source_project], + source_branch: params[:source_branch], + target_project: params[:target_project], + target_branch: params[:target_branch], + character_limit: params[:character_limit] + ) + .and_return("diff") + + expect(template.send(:extracted_diff)).to eq("diff") end end end diff --git a/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb b/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb index 3bcee7ed061d55..49b1457f56c34a 100644 --- a/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb +++ b/ee/spec/lib/gitlab/llm/utils/merge_request_tool_spec.rb @@ -3,62 +3,74 @@ require 'spec_helper' RSpec.describe Gitlab::Llm::Utils::MergeRequestTool, feature_category: :ai_abstraction_layer do - describe '.extract_diff' do - let_it_be(:source_project) { create(:project) } - let(:source_branch) { 'main' } - let_it_be(:target_project) { create(:project) } - let(:target_branch) { 'feature' } - let(:character_limit) { 1000 } - - subject(:merge_tool_result) do - described_class.extract_diff(source_project: source_project, source_branch: source_branch, - target_project: target_project, target_branch: target_branch, character_limit: character_limit) + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.owner } + + let(:source_project) { project } + let(:target_project) { project } + let(:source_branch) { 'feature' } + let(:target_branch) { 'master' } + + let(:character_limit) { 1000 } + + let(:arguments) do + { + source_project: source_project, + target_project: target_project, + source_branch: source_branch, + target_branch: target_branch, + character_limit: character_limit + } + end + + context "when there is a diff with an edge case" do + let(:good_diff) { { diff: "@@ -0,0 +1 @@hellothere\n+🌚\n" } } + let(:compare) { instance_double(Compare) } + + before do + allow(CompareService).to receive_message_chain(:new, :execute).and_return(compare) end - context 'when compare service returns a result' do - let(:compare) { instance_double(:compare, raw_diffs: raw_diffs) } - let(:raw_diffs) { [raw_diff_1, raw_diff_2] } - let(:raw_diff_1) do - instance_double(:raw_diff, diff: "diff --git a/file1.rb b/file1.rb\n+foo\n-bar", old_path: 'file1.rb', - new_path: 'file1.rb', has_binary_notice?: false) + context 'when a diff is not encoded with UTF-8' do + let(:other_diff) do + { diff: "@@ -1 +1 @@\n-This should not be in the prompt\n+#{(0..255).map(&:chr).join}\n" } end - let(:raw_diff_2) do - instance_double(:raw_diff, diff: "diff --git a/file2.rb b/file2.rb\n+baz\n-qux", old_path: 'file2.rb', - new_path: 'file2.rb', has_binary_notice?: false) - end + let(:diff_files) { Gitlab::Git::DiffCollection.new([good_diff, other_diff]) } - before do - allow(CompareService).to receive(:new).and_return(instance_double(:compare_service, execute: compare)) + it 'does not raise any error and not contain the non-UTF diff' do + allow(compare).to receive(:raw_diffs).and_return(diff_files) + extracted_diff = described_class.extract_diff(**arguments) + expect(extracted_diff).to include("hellothere") + expect(extracted_diff).not_to include("This should not be in the prompt") end + end - it 'returns the formatted diff' do - expected_diff = "--- file1.rb\n+++ file1.rb\n+foo\n-bar\n\n--- file2.rb\n+++ file2.rb\n+baz\n-qux" - expect(merge_tool_result).to eq(expected_diff) + context 'when a diff contains the binary notice' do + let(:binary_message) { Gitlab::Git::Diff.binary_message('a', 'b') } + let(:other_diff) { { diff: binary_message } } + let(:diff_files) { Gitlab::Git::DiffCollection.new([good_diff, other_diff]) } + + it 'does not contain the binary diff' do + allow(compare).to receive(:raw_diffs).and_return(diff_files) + extracted_diff = described_class.extract_diff(**arguments) + + expect(extracted_diff).to include("hellothere") + expect(extracted_diff).not_to include(binary_message) end end - context 'when compare service returns nil' do + context 'when extracted diff is blank' do + let(:diff_files) { Gitlab::Git::DiffCollection.new([good_diff]) } + before do - allow(CompareService).to receive(:new).and_return(instance_double(:compare_service, execute: nil)) + allow(CompareService).to receive_message_chain(:new, :execute).and_return(nil) end it 'returns nil' do - expect(merge_tool_result).to be_nil + extracted_diff = described_class.extract_diff(**arguments) + expect(extracted_diff).to be_nil end end end - - describe '.diff_output' do - let(:old_path) { 'file.rb' } - let(:new_path) { 'file.rb' } - let(:diff) { "+foo\n-bar" } - - subject(:merge_request_diff) { described_class.diff_output(old_path, new_path, diff) } - - it 'returns the formatted diff' do - expected_diff = "--- file.rb\n+++ file.rb\n+foo\n-bar" - expect(merge_request_diff).to eq(expected_diff) - end - end end diff --git a/ee/spec/models/ai/ai_resource/merge_request_spec.rb b/ee/spec/models/ai/ai_resource/merge_request_spec.rb index a33abe15d336a6..abacf79725dbea 100644 --- a/ee/spec/models/ai/ai_resource/merge_request_spec.rb +++ b/ee/spec/models/ai/ai_resource/merge_request_spec.rb @@ -25,15 +25,15 @@ describe '#current_page_sentence' do it 'returns prompt' do - expect(wrapped_merge_request.current_page_sentence).to - include(" utilize it instead of using the 'MergeRequestReader' tool.") + expect(wrapped_merge_request.current_page_sentence).to include("utilize it instead of using" \ + "the 'MergeRequestReader' tool.") end end describe '#current_page_short_description' do it 'returns prompt' do - expect(wrapped_merge_request.current_page_short_description).to - include("The title of the merge request is '#{merge_request.title}'.") + expect(wrapped_merge_request.current_page_short_description).to include("The title of the merge request" \ + "is '#{merge_request.title}'.") end end end -- GitLab From 2873d0acaddf05a95dd407fe89ada675337efc97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 27 May 2024 13:09:55 +0200 Subject: [PATCH 15/32] Fix failing specs --- ee/app/models/ai/ai_resource/merge_request.rb | 2 +- ee/spec/models/ai/ai_resource/merge_request_spec.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ee/app/models/ai/ai_resource/merge_request.rb b/ee/app/models/ai/ai_resource/merge_request.rb index dc6b7f52b08911..490f2a0b2a2938 100644 --- a/ee/app/models/ai/ai_resource/merge_request.rb +++ b/ee/app/models/ai/ai_resource/merge_request.rb @@ -23,7 +23,7 @@ def current_page_sentence def current_page_short_description <<~SENTENCE - The user is currently on a page that displays a merge request with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the merge request is '#{resource.title}'. Remember to use the 'MergeRequestReader' tool if they ask a question about the MergeRequest. + The user is currently on a page that displays a merge request with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the merge request is '#{resource.title}'. Remember to use the 'MergeRequestReader' tool if they ask a question about the Merge Request. SENTENCE end end diff --git a/ee/spec/models/ai/ai_resource/merge_request_spec.rb b/ee/spec/models/ai/ai_resource/merge_request_spec.rb index abacf79725dbea..270e0d3e64260f 100644 --- a/ee/spec/models/ai/ai_resource/merge_request_spec.rb +++ b/ee/spec/models/ai/ai_resource/merge_request_spec.rb @@ -25,15 +25,15 @@ describe '#current_page_sentence' do it 'returns prompt' do - expect(wrapped_merge_request.current_page_sentence).to include("utilize it instead of using" \ - "the 'MergeRequestReader' tool.") + expect(wrapped_merge_request.current_page_sentence) + .to include("utilize it instead of using the 'MergeRequestReader' tool.") end end describe '#current_page_short_description' do it 'returns prompt' do - expect(wrapped_merge_request.current_page_short_description).to include("The title of the merge request" \ - "is '#{merge_request.title}'.") + expect(wrapped_merge_request.current_page_short_description) + .to include("The title of the merge request is '#{merge_request.title}'.") end end end -- GitLab From aab7d487a62a8def20c56004b0e44424e8c691ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 27 May 2024 18:12:03 +0200 Subject: [PATCH 16/32] Refactor to keep everything in EE directory --- app/serializers/merge_request_ai_entity.rb | 17 -------- app/serializers/merge_request_serializer.rb | 41 ++++++++++--------- .../serializers/ee/merge_request/ai_entity.rb | 21 ++++++++++ .../ee/merge_request_serializer.rb | 16 ++++++++ .../schemas/entities/merge_request_ai.json | 16 ++++++++ .../merge_request/ai_entity_spec.rb | 2 +- .../merge_request_serializer_spec.rb | 24 +++++++++++ .../schemas/entities/merge_request_ai.json | 11 ----- 8 files changed, 99 insertions(+), 49 deletions(-) delete mode 100644 app/serializers/merge_request_ai_entity.rb create mode 100644 ee/app/serializers/ee/merge_request/ai_entity.rb create mode 100644 ee/app/serializers/ee/merge_request_serializer.rb create mode 100644 ee/spec/fixtures/api/schemas/entities/merge_request_ai.json rename spec/serializers/merge_request_ai_entity_spec.rb => ee/spec/serializers/merge_request/ai_entity_spec.rb (96%) create mode 100644 ee/spec/serializers/merge_request_serializer_spec.rb delete mode 100644 spec/fixtures/api/schemas/entities/merge_request_ai.json diff --git a/app/serializers/merge_request_ai_entity.rb b/app/serializers/merge_request_ai_entity.rb deleted file mode 100644 index f62e1be21db5bd..00000000000000 --- a/app/serializers/merge_request_ai_entity.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -class MergeRequestAiEntity < ::API::Entities::MergeRequestBasic # rubocop:disable Gitlab/NamespacedClass -- serializer doesn't need module - expose :mr_comments do |_mr, options| - options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit] / 2) - end - - expose :diff do |mr, options| - Gitlab::Llm::Utils::MergeRequestTool.extract_diff( - source_project: mr.source_project, - source_branch: mr.source_branch, - target_project: mr.target_project, - target_branch: mr.target_branch, - character_limit: options[:notes_limit] / 2 - ) - end -end diff --git a/app/serializers/merge_request_serializer.rb b/app/serializers/merge_request_serializer.rb index 5fe4978f76db40..2805a9a7ccc7d5 100644 --- a/app/serializers/merge_request_serializer.rb +++ b/app/serializers/merge_request_serializer.rb @@ -5,29 +5,30 @@ class MergeRequestSerializer < BaseSerializer # to serialize the `merge_request` based on `serializer` key in `opts` param. # Hence, `entity` doesn't need to be declared on the class scope. def represent(merge_request, opts = {}, entity = nil) - entity ||= - case opts[:serializer] - when 'sidebar' - MergeRequestSidebarBasicEntity - when 'sidebar_extras' - MergeRequestSidebarExtrasEntity - when 'basic' - MergeRequestBasicEntity - when 'noteable' - MergeRequestNoteableEntity - when 'poll_cached_widget' - MergeRequestPollCachedWidgetEntity - when 'poll_widget' - MergeRequestPollWidgetEntity - when 'ai' - MergeRequestAiEntity - else - # fallback to widget for old poll requests without `serializer` set - MergeRequestWidgetEntity - end + entity ||= identified_entity(opts) super(merge_request, opts, entity) end + + def identified_entity(opts) + case opts[:serializer] + when 'sidebar' + MergeRequestSidebarBasicEntity + when 'sidebar_extras' + MergeRequestSidebarExtrasEntity + when 'basic' + MergeRequestBasicEntity + when 'noteable' + MergeRequestNoteableEntity + when 'poll_cached_widget' + MergeRequestPollCachedWidgetEntity + when 'poll_widget' + MergeRequestPollWidgetEntity + else + # fallback to widget for old poll requests without `serializer` set + MergeRequestWidgetEntity + end + end end MergeRequestSerializer.prepend_mod diff --git a/ee/app/serializers/ee/merge_request/ai_entity.rb b/ee/app/serializers/ee/merge_request/ai_entity.rb new file mode 100644 index 00000000000000..cd80e008f4ac1d --- /dev/null +++ b/ee/app/serializers/ee/merge_request/ai_entity.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module EE + module MergeRequest + class AiEntity < ::API::Entities::MergeRequestBasic + expose :mr_comments do |_mr, options| + options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit] / 2) + end + + expose :diff do |mr, options| + ::Gitlab::Llm::Utils::MergeRequestTool.extract_diff( + source_project: mr.source_project, + source_branch: mr.source_branch, + target_project: mr.target_project, + target_branch: mr.target_branch, + character_limit: options[:notes_limit] / 2 + ) + end + end + end +end diff --git a/ee/app/serializers/ee/merge_request_serializer.rb b/ee/app/serializers/ee/merge_request_serializer.rb new file mode 100644 index 00000000000000..a7b8f58c84b6ff --- /dev/null +++ b/ee/app/serializers/ee/merge_request_serializer.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module MergeRequestSerializer + extend ::Gitlab::Utils::Override + + override :identified_entity + def identified_entity(opts) + if opts[:serializer] == 'ai' + MergeRequest::AiEntity + else + super(merge_request, opts, entity) + end + end + end +end diff --git a/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json b/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json new file mode 100644 index 00000000000000..5ea73c5e21d14d --- /dev/null +++ b/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json @@ -0,0 +1,16 @@ +{ + "type": "object", + "allOf": [ + { "$ref": "./merge_request_noteable.json" }, + { + "properties" : { + "mr_comments": { + "type": "array" + }, + "diff": { + "type": "string" + } + } + } + ] +} diff --git a/spec/serializers/merge_request_ai_entity_spec.rb b/ee/spec/serializers/merge_request/ai_entity_spec.rb similarity index 96% rename from spec/serializers/merge_request_ai_entity_spec.rb rename to ee/spec/serializers/merge_request/ai_entity_spec.rb index afba6a51858447..5e522107ddbefd 100644 --- a/spec/serializers/merge_request_ai_entity_spec.rb +++ b/ee/spec/serializers/merge_request/ai_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MergeRequestAiEntity, feature_category: :ai_abstraction_layer do +RSpec.describe MergeRequest::AiEntity, feature_category: :ai_abstraction_layer do let_it_be(:user) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it let_it_be(:merge_request) { create(:merge_request) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it let(:notes_limit) { 1000 } diff --git a/ee/spec/serializers/merge_request_serializer_spec.rb b/ee/spec/serializers/merge_request_serializer_spec.rb new file mode 100644 index 00000000000000..5bba64db01d596 --- /dev/null +++ b/ee/spec/serializers/merge_request_serializer_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequestSerializer, feature_category: :code_review_workflow do + let_it_be(:user) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it + let_it_be(:merge_request) { create(:merge_request, description: "Description") } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it + let(:serializer) { 'ai' } + + let(:json_entity) do + described_class.new(current_user: user) + .represent(merge_request, + serializer: serializer, + notes_limit: 2, + resource: Ai::AiResource::MergeRequest.new(merge_request)) + .with_indifferent_access + end + + context 'when serializing merge request for ai' do + it 'matches merge_request_ai json schema' do + expect(json_entity).to match_schema('entities/merge_request_ai') + end + end +end diff --git a/spec/fixtures/api/schemas/entities/merge_request_ai.json b/spec/fixtures/api/schemas/entities/merge_request_ai.json deleted file mode 100644 index a56fa2075bc141..00000000000000 --- a/spec/fixtures/api/schemas/entities/merge_request_ai.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "type": "object", - "properties": { - "mr_comments": { - "type": "array" - }, - "diff": { - "type": "string" - } - } -} -- GitLab From 7d9dac32e089af4c1205668882d24690d96f7de9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 27 May 2024 18:13:43 +0200 Subject: [PATCH 17/32] Prettify json --- ee/spec/fixtures/api/schemas/entities/merge_request_ai.json | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json b/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json index 5ea73c5e21d14d..b3bcd417636089 100644 --- a/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json +++ b/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json @@ -1,9 +1,11 @@ { "type": "object", "allOf": [ - { "$ref": "./merge_request_noteable.json" }, { - "properties" : { + "$ref": "./merge_request_noteable.json" + }, + { + "properties": { "mr_comments": { "type": "array" }, -- GitLab From 103b60b69818def0e9329825e3d2acbf53a58905 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 28 May 2024 12:33:41 +0200 Subject: [PATCH 18/32] Fix failing specs --- ee/app/serializers/ee/merge_request_serializer.rb | 2 +- ee/spec/serializers/merge_request_serializer_spec.rb | 1 + spec/serializers/merge_request_serializer_spec.rb | 8 -------- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/ee/app/serializers/ee/merge_request_serializer.rb b/ee/app/serializers/ee/merge_request_serializer.rb index a7b8f58c84b6ff..cf81735f75418f 100644 --- a/ee/app/serializers/ee/merge_request_serializer.rb +++ b/ee/app/serializers/ee/merge_request_serializer.rb @@ -9,7 +9,7 @@ def identified_entity(opts) if opts[:serializer] == 'ai' MergeRequest::AiEntity else - super(merge_request, opts, entity) + super(opts) end end end diff --git a/ee/spec/serializers/merge_request_serializer_spec.rb b/ee/spec/serializers/merge_request_serializer_spec.rb index 5bba64db01d596..d15623b3736957 100644 --- a/ee/spec/serializers/merge_request_serializer_spec.rb +++ b/ee/spec/serializers/merge_request_serializer_spec.rb @@ -4,6 +4,7 @@ RSpec.describe MergeRequestSerializer, feature_category: :code_review_workflow do let_it_be(:user) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it + let_it_be(:merge_request) { create(:merge_request, description: "Description") } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it let(:serializer) { 'ai' } diff --git a/spec/serializers/merge_request_serializer_spec.rb b/spec/serializers/merge_request_serializer_spec.rb index a8e67f33c2e816..6baf5cae06a50e 100644 --- a/spec/serializers/merge_request_serializer_spec.rb +++ b/spec/serializers/merge_request_serializer_spec.rb @@ -85,14 +85,6 @@ end end - context 'poll merge request ai serialization' do - let(:serializer) { 'merge_request_ai' } - - it 'matches merge_request_ai json schema' do - expect(json_entity).to match_schema('entities/merge_request_ai') - end - end - context 'no serializer' do let(:serializer) { nil } -- GitLab From d0a95c8317546f50e3cf0b8a997b52ca46ae2221 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 28 May 2024 14:55:58 +0200 Subject: [PATCH 19/32] Simplify specs --- .../serializers/ee/merge_request/ai_entity.rb | 2 +- .../api/schemas/entities/merge_request_ai.json | 18 ------------------ .../merge_request_serializer_spec.rb | 5 ++--- 3 files changed, 3 insertions(+), 22 deletions(-) delete mode 100644 ee/spec/fixtures/api/schemas/entities/merge_request_ai.json diff --git a/ee/app/serializers/ee/merge_request/ai_entity.rb b/ee/app/serializers/ee/merge_request/ai_entity.rb index cd80e008f4ac1d..751bfefecb36d9 100644 --- a/ee/app/serializers/ee/merge_request/ai_entity.rb +++ b/ee/app/serializers/ee/merge_request/ai_entity.rb @@ -2,7 +2,7 @@ module EE module MergeRequest - class AiEntity < ::API::Entities::MergeRequestBasic + class AiEntity < ::API::Entities::MergeRequest expose :mr_comments do |_mr, options| options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit] / 2) end diff --git a/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json b/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json deleted file mode 100644 index b3bcd417636089..00000000000000 --- a/ee/spec/fixtures/api/schemas/entities/merge_request_ai.json +++ /dev/null @@ -1,18 +0,0 @@ -{ - "type": "object", - "allOf": [ - { - "$ref": "./merge_request_noteable.json" - }, - { - "properties": { - "mr_comments": { - "type": "array" - }, - "diff": { - "type": "string" - } - } - } - ] -} diff --git a/ee/spec/serializers/merge_request_serializer_spec.rb b/ee/spec/serializers/merge_request_serializer_spec.rb index d15623b3736957..880fcc02a7c9ae 100644 --- a/ee/spec/serializers/merge_request_serializer_spec.rb +++ b/ee/spec/serializers/merge_request_serializer_spec.rb @@ -4,7 +4,6 @@ RSpec.describe MergeRequestSerializer, feature_category: :code_review_workflow do let_it_be(:user) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it - let_it_be(:merge_request) { create(:merge_request, description: "Description") } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it let(:serializer) { 'ai' } @@ -18,8 +17,8 @@ end context 'when serializing merge request for ai' do - it 'matches merge_request_ai json schema' do - expect(json_entity).to match_schema('entities/merge_request_ai') + it 'returns ai related data' do + expect(json_entity.keys).to include("mr_comments", "diff") end end end -- GitLab From 3d8e004694c2c4123d8f7411fb8c135281d17e4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 28 May 2024 15:08:58 +0200 Subject: [PATCH 20/32] Remove obsolete change --- ee/app/workers/llm/completion_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/workers/llm/completion_worker.rb b/ee/app/workers/llm/completion_worker.rb index 3d2dd9052b6666..258fe03212dc9c 100644 --- a/ee/app/workers/llm/completion_worker.rb +++ b/ee/app/workers/llm/completion_worker.rb @@ -30,7 +30,7 @@ def deserialize_message(message_hash, options) end def perform_for(message, options = {}) - perform_inline(serialize_message(message), options) + perform_async(serialize_message(message), options) end end -- GitLab From 8ba4687395ebcf234125ca257b051185beb148f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Wed, 29 May 2024 12:32:32 +0200 Subject: [PATCH 21/32] Block by feature flag the current page part for merge requests --- ee/app/models/ai/ai_resource/epic.rb | 4 ++-- ee/app/models/ai/ai_resource/issue.rb | 4 ++-- ee/app/models/ai/ai_resource/merge_request.rb | 8 ++++++-- ee/app/workers/llm/completion_worker.rb | 2 +- .../gitlab/llm/chain/agents/zero_shot/executor.rb | 13 +++++++++++-- ee/lib/gitlab/llm/chain/gitlab_context.rb | 4 ++-- 6 files changed, 24 insertions(+), 11 deletions(-) diff --git a/ee/app/models/ai/ai_resource/epic.rb b/ee/app/models/ai/ai_resource/epic.rb index f176749e9f5b46..5fcf4f3073fdf7 100644 --- a/ee/app/models/ai/ai_resource/epic.rb +++ b/ee/app/models/ai/ai_resource/epic.rb @@ -15,13 +15,13 @@ def serialize_for_ai(user:, content_limit:) }) end - def current_page_sentence + def current_page_sentence(_current_user: nil) <<~SENTENCE The user is currently on a page that displays an epic with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'EpicReader' tool. SENTENCE end - def current_page_short_description + def current_page_short_description(_current_user: nil) <<~SENTENCE The user is currently on a page that displays an epic with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the epic is '#{resource.title}'. Remember to use the 'EpicReader' tool if they ask a question about the epic. SENTENCE diff --git a/ee/app/models/ai/ai_resource/issue.rb b/ee/app/models/ai/ai_resource/issue.rb index 77d209be3b70c6..af580565811741 100644 --- a/ee/app/models/ai/ai_resource/issue.rb +++ b/ee/app/models/ai/ai_resource/issue.rb @@ -15,13 +15,13 @@ def serialize_for_ai(user:, content_limit:) }) end - def current_page_sentence + def current_page_sentence(_current_user: nil) <<~SENTENCE The user is currently on a page that displays an issue with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'IssueReader' tool. SENTENCE end - def current_page_short_description + def current_page_short_description(_current_user: nil) <<~SENTENCE The user is currently on a page that displays an issue with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the issue is '#{resource.title}'. Remember to use the 'IssueReader' tool if they ask a question about the issue. SENTENCE diff --git a/ee/app/models/ai/ai_resource/merge_request.rb b/ee/app/models/ai/ai_resource/merge_request.rb index 490f2a0b2a2938..e41815e6f139a2 100644 --- a/ee/app/models/ai/ai_resource/merge_request.rb +++ b/ee/app/models/ai/ai_resource/merge_request.rb @@ -15,13 +15,17 @@ def serialize_for_ai(user:, content_limit:) }) end - def current_page_sentence + def current_page_sentence(current_user: nil) + return '' unless Feature.enabled?(:ai_merge_request_reader_for_chat, current_user) + <<~SENTENCE The user is currently on a page that displays a merge request with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'MergeRequestReader' tool. SENTENCE end - def current_page_short_description + def current_page_short_description(current_user: nil) + return '' unless Feature.enabled?(:ai_merge_request_reader_for_chat, current_user) + <<~SENTENCE The user is currently on a page that displays a merge request with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the merge request is '#{resource.title}'. Remember to use the 'MergeRequestReader' tool if they ask a question about the Merge Request. SENTENCE diff --git a/ee/app/workers/llm/completion_worker.rb b/ee/app/workers/llm/completion_worker.rb index 258fe03212dc9c..3d2dd9052b6666 100644 --- a/ee/app/workers/llm/completion_worker.rb +++ b/ee/app/workers/llm/completion_worker.rb @@ -30,7 +30,7 @@ def deserialize_message(message_hash, options) end def perform_for(message, options = {}) - perform_async(serialize_message(message), options) + perform_inline(serialize_message(message), options) end end diff --git a/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb b/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb index c565d0ee651a5a..e4a889c2f94bfd 100644 --- a/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb +++ b/ee/lib/gitlab/llm/chain/agents/zero_shot/executor.rb @@ -113,7 +113,8 @@ def options current_resource: current_resource, source_template: source_template, current_code: current_code, - resources: available_resources_names + resources: available_resources_names, + unavailable_resources: unavailable_resources_names } end @@ -147,6 +148,14 @@ def available_resources_names end strong_memoize_attr :available_resources_names + def unavailable_resources_names + resources = %w[Pipelines Vulnerabilities] + resources << 'Merge Requests' unless Feature.enabled?(:ai_merge_request_reader_for_chat, + context.current_user) + + resources.join(', ') + end + def prompt_version return CUSTOM_AGENT_PROMPT_TEMPLATE if context.agent_version @@ -243,7 +252,7 @@ def source_template You have access to the following GitLab resources: %s. You also have access to all information that can be helpful to someone working in software development of any kind. - At the moment, you do not have access to the following GitLab resources: Pipelines, Vulnerabilities. + At the moment, you do not have access to the following GitLab resources: %s. %s diff --git a/ee/lib/gitlab/llm/chain/gitlab_context.rb b/ee/lib/gitlab/llm/chain/gitlab_context.rb index e5970f0e81efb9..21e8cbd410dab4 100644 --- a/ee/lib/gitlab/llm/chain/gitlab_context.rb +++ b/ee/lib/gitlab/llm/chain/gitlab_context.rb @@ -23,11 +23,11 @@ def initialize( end def current_page_sentence - authorized_resource&.current_page_sentence + authorized_resource&.current_page_sentence(current_user: current_user) end def current_page_short_description - authorized_resource&.current_page_short_description + authorized_resource&.current_page_short_description(current_user: current_user) end def resource_serialized(content_limit:) -- GitLab From 299d7d9af9399afcca57741e3ec478a895a807e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Wed, 29 May 2024 14:13:51 +0200 Subject: [PATCH 22/32] Add better serving of feature flag for merge requests --- ee/app/models/ai/ai_resource/epic.rb | 4 ++-- ee/app/models/ai/ai_resource/issue.rb | 4 ++-- ee/app/workers/llm/completion_worker.rb | 2 +- .../zero_shot/prompts/anthropic_spec.rb | 2 ++ .../ai/ai_resource/merge_request_spec.rb | 22 +++++++++++++++++++ 5 files changed, 29 insertions(+), 5 deletions(-) diff --git a/ee/app/models/ai/ai_resource/epic.rb b/ee/app/models/ai/ai_resource/epic.rb index 5fcf4f3073fdf7..6aa5d0bb2141ef 100644 --- a/ee/app/models/ai/ai_resource/epic.rb +++ b/ee/app/models/ai/ai_resource/epic.rb @@ -15,13 +15,13 @@ def serialize_for_ai(user:, content_limit:) }) end - def current_page_sentence(_current_user: nil) + def current_page_sentence(*) <<~SENTENCE The user is currently on a page that displays an epic with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'EpicReader' tool. SENTENCE end - def current_page_short_description(_current_user: nil) + def current_page_short_description(*) <<~SENTENCE The user is currently on a page that displays an epic with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the epic is '#{resource.title}'. Remember to use the 'EpicReader' tool if they ask a question about the epic. SENTENCE diff --git a/ee/app/models/ai/ai_resource/issue.rb b/ee/app/models/ai/ai_resource/issue.rb index af580565811741..260618a4c9c157 100644 --- a/ee/app/models/ai/ai_resource/issue.rb +++ b/ee/app/models/ai/ai_resource/issue.rb @@ -15,13 +15,13 @@ def serialize_for_ai(user:, content_limit:) }) end - def current_page_sentence(_current_user: nil) + def current_page_sentence(*) <<~SENTENCE The user is currently on a page that displays an issue with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'IssueReader' tool. SENTENCE end - def current_page_short_description(_current_user: nil) + def current_page_short_description(*) <<~SENTENCE The user is currently on a page that displays an issue with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the issue is '#{resource.title}'. Remember to use the 'IssueReader' tool if they ask a question about the issue. SENTENCE diff --git a/ee/app/workers/llm/completion_worker.rb b/ee/app/workers/llm/completion_worker.rb index 3d2dd9052b6666..258fe03212dc9c 100644 --- a/ee/app/workers/llm/completion_worker.rb +++ b/ee/app/workers/llm/completion_worker.rb @@ -30,7 +30,7 @@ def deserialize_message(message_hash, options) end def perform_for(message, options = {}) - perform_inline(serialize_message(message), options) + perform_async(serialize_message(message), options) end end diff --git a/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb b/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb index 0451e57a100292..e2835fc0e60857 100644 --- a/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb @@ -31,6 +31,8 @@ zero_shot_prompt: zero_shot_prompt, system_prompt: system_prompt, source_template: "source template" + system_prompt: system_prompt, + unavailable_resources: '' } end diff --git a/ee/spec/models/ai/ai_resource/merge_request_spec.rb b/ee/spec/models/ai/ai_resource/merge_request_spec.rb index 270e0d3e64260f..16eb29818bafff 100644 --- a/ee/spec/models/ai/ai_resource/merge_request_spec.rb +++ b/ee/spec/models/ai/ai_resource/merge_request_spec.rb @@ -28,6 +28,17 @@ expect(wrapped_merge_request.current_page_sentence) .to include("utilize it instead of using the 'MergeRequestReader' tool.") end + + context 'with mr for chat feature flag disabled' do + before do + stub_feature_flags(ai_merge_request_reader_for_chat: false) + end + + it 'returns empty string' do + expect(wrapped_merge_request.current_page_sentence) + .to eq("") + end + end end describe '#current_page_short_description' do @@ -35,5 +46,16 @@ expect(wrapped_merge_request.current_page_short_description) .to include("The title of the merge request is '#{merge_request.title}'.") end + + context 'with mr for chat feature flag disabled' do + before do + stub_feature_flags(ai_merge_request_reader_for_chat: false) + end + + it 'returns empty string' do + expect(wrapped_merge_request.current_page_short_description) + .to eq("") + end + end end end -- GitLab From 1b891887f000eebea61c9f31af9980ff5d980b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Wed, 29 May 2024 14:35:28 +0200 Subject: [PATCH 23/32] Fix bad rebase --- .../llm/chain/agents/zero_shot/prompts/anthropic_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb b/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb index e2835fc0e60857..48db69fc026350 100644 --- a/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb @@ -30,9 +30,8 @@ current_user: user, zero_shot_prompt: zero_shot_prompt, system_prompt: system_prompt, + unavailable_resources: '', source_template: "source template" - system_prompt: system_prompt, - unavailable_resources: '' } end -- GitLab From e18eaad3ab84db172047771e79cec2b15af22471 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 30 May 2024 09:19:39 +0200 Subject: [PATCH 24/32] Fix undercoverage --- .../gitlab/llm/chain/gitlab_context_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb b/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb index 349acab2fbd4b6..03bf24160199d2 100644 --- a/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb @@ -92,4 +92,22 @@ end end end + + describe '#current_page_description' do + context 'with an unauthorized resource' do + let(:resource) { create(:issue) } + + it 'returns nil' do + expect(context.current_page_sentence).to be_nil + end + end + + context 'with an authorized resource' do + let(:resource) { create(:issue, project: project) } + + it 'returns sentence about the resource' do + expect(context.current_page_sentence).to include("The user is currently on a page that displays an issue") + end + end + end end -- GitLab From 5242a74c4cd09e2c2945bca8373d2c946341ff30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 31 May 2024 18:48:48 +0200 Subject: [PATCH 25/32] Refactor ai resources classes --- ee/app/models/ai/ai_resource/base_ai_resource.rb | 7 ++++--- ee/app/models/ai/ai_resource/concerns/noteable.rb | 4 ++-- ee/app/models/ai/ai_resource/epic.rb | 10 +++++----- ee/app/models/ai/ai_resource/issue.rb | 10 +++++----- ee/app/models/ai/ai_resource/merge_request.rb | 10 +++++----- ee/app/serializers/ee/issue_ai_entity.rb | 2 +- ee/app/serializers/ee/merge_request/ai_entity.rb | 2 +- ee/app/serializers/epic_ai_entity.rb | 2 +- ee/lib/gitlab/llm/chain/gitlab_context.rb | 12 +++++------- ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb | 4 ++-- .../llm/chain/tools/epic_reader/executor_spec.rb | 3 +-- .../llm/chain/tools/issue_reader/executor_spec.rb | 3 +-- .../tools/merge_request_reader/executor_spec.rb | 3 +-- .../llm/completions/chat_real_requests_spec.rb | 1 - .../models/ai/ai_resource/base_ai_resource_spec.rb | 2 +- .../models/ai/ai_resource/concerns/noteable_spec.rb | 2 +- ee/spec/models/ai/ai_resource/epic_spec.rb | 4 ++-- ee/spec/models/ai/ai_resource/issue_spec.rb | 4 ++-- ee/spec/models/ai/ai_resource/merge_request_spec.rb | 4 ++-- ee/spec/serializers/epic_serializer_spec.rb | 2 +- ee/spec/serializers/issue_serializer_spec.rb | 2 +- ee/spec/serializers/merge_request/ai_entity_spec.rb | 2 +- ee/spec/serializers/merge_request_serializer_spec.rb | 2 +- 23 files changed, 46 insertions(+), 51 deletions(-) diff --git a/ee/app/models/ai/ai_resource/base_ai_resource.rb b/ee/app/models/ai/ai_resource/base_ai_resource.rb index 15992bb32d339d..6935c700493b3f 100644 --- a/ee/app/models/ai/ai_resource/base_ai_resource.rb +++ b/ee/app/models/ai/ai_resource/base_ai_resource.rb @@ -3,13 +3,14 @@ module Ai module AiResource class BaseAiResource - attr_reader :resource + attr_reader :resource, :current_user - def initialize(resource) + def initialize(user, resource) @resource = resource + @current_user = user end - def serialize_for_ai(_user:, _content_limit:) + def serialize_for_ai(_content_limit:) raise NotImplementedError end end diff --git a/ee/app/models/ai/ai_resource/concerns/noteable.rb b/ee/app/models/ai/ai_resource/concerns/noteable.rb index 0c64be121b235c..a0f2ad0ec41bc6 100644 --- a/ee/app/models/ai/ai_resource/concerns/noteable.rb +++ b/ee/app/models/ai/ai_resource/concerns/noteable.rb @@ -5,8 +5,8 @@ module AiResource module Concerns module Noteable extend ActiveSupport::Concern - def notes_with_limit(user, notes_limit:) - limited_notes = Ai::NotesForAiFinder.new(user, resource: resource).execute + def notes_with_limit(notes_limit:) + limited_notes = Ai::NotesForAiFinder.new(current_user, resource: resource).execute return [] if limited_notes.empty? diff --git a/ee/app/models/ai/ai_resource/epic.rb b/ee/app/models/ai/ai_resource/epic.rb index 6aa5d0bb2141ef..6a5a2d32fc130b 100644 --- a/ee/app/models/ai/ai_resource/epic.rb +++ b/ee/app/models/ai/ai_resource/epic.rb @@ -5,23 +5,23 @@ module AiResource class Epic < Ai::AiResource::BaseAiResource include Ai::AiResource::Concerns::Noteable - def serialize_for_ai(user:, content_limit:) - ::EpicSerializer.new(current_user: user) # rubocop: disable CodeReuse/Serializer + def serialize_for_ai(content_limit:) + ::EpicSerializer.new(current_user: current_user) # rubocop: disable CodeReuse/Serializer .represent(resource, { - user: user, + user: current_user, notes_limit: content_limit, serializer: 'ai', resource: self }) end - def current_page_sentence(*) + def current_page_sentence <<~SENTENCE The user is currently on a page that displays an epic with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'EpicReader' tool. SENTENCE end - def current_page_short_description(*) + def current_page_short_description <<~SENTENCE The user is currently on a page that displays an epic with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the epic is '#{resource.title}'. Remember to use the 'EpicReader' tool if they ask a question about the epic. SENTENCE diff --git a/ee/app/models/ai/ai_resource/issue.rb b/ee/app/models/ai/ai_resource/issue.rb index 260618a4c9c157..2c2232f005e151 100644 --- a/ee/app/models/ai/ai_resource/issue.rb +++ b/ee/app/models/ai/ai_resource/issue.rb @@ -5,23 +5,23 @@ module AiResource class Issue < Ai::AiResource::BaseAiResource include Ai::AiResource::Concerns::Noteable - def serialize_for_ai(user:, content_limit:) - ::IssueSerializer.new(current_user: user, project: resource.project) # rubocop: disable CodeReuse/Serializer + def serialize_for_ai(content_limit:) + ::IssueSerializer.new(current_user: current_user, project: resource.project) # rubocop: disable CodeReuse/Serializer .represent(resource, { - user: user, + user: current_user, notes_limit: content_limit, serializer: 'ai', resource: self }) end - def current_page_sentence(*) + def current_page_sentence <<~SENTENCE The user is currently on a page that displays an issue with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The data is provided in tags, and if it is sufficient in answering the question, utilize it instead of using the 'IssueReader' tool. SENTENCE end - def current_page_short_description(*) + def current_page_short_description <<~SENTENCE The user is currently on a page that displays an issue with a description, comments, etc., which the user might refer to, for example, as 'current', 'this' or 'that'. The title of the issue is '#{resource.title}'. Remember to use the 'IssueReader' tool if they ask a question about the issue. SENTENCE diff --git a/ee/app/models/ai/ai_resource/merge_request.rb b/ee/app/models/ai/ai_resource/merge_request.rb index e41815e6f139a2..57f79215f7e848 100644 --- a/ee/app/models/ai/ai_resource/merge_request.rb +++ b/ee/app/models/ai/ai_resource/merge_request.rb @@ -5,17 +5,17 @@ module AiResource class MergeRequest < Ai::AiResource::BaseAiResource include Ai::AiResource::Concerns::Noteable - def serialize_for_ai(user:, content_limit:) - ::MergeRequestSerializer.new(current_user: user) # rubocop: disable CodeReuse/Serializer -- existing serializer + def serialize_for_ai(content_limit:) + ::MergeRequestSerializer.new(current_user: current_user) # rubocop: disable CodeReuse/Serializer -- existing serializer .represent(resource, { - user: user, + user: current_user, notes_limit: content_limit, serializer: 'ai', resource: self }) end - def current_page_sentence(current_user: nil) + def current_page_sentence return '' unless Feature.enabled?(:ai_merge_request_reader_for_chat, current_user) <<~SENTENCE @@ -23,7 +23,7 @@ def current_page_sentence(current_user: nil) SENTENCE end - def current_page_short_description(current_user: nil) + def current_page_short_description return '' unless Feature.enabled?(:ai_merge_request_reader_for_chat, current_user) <<~SENTENCE diff --git a/ee/app/serializers/ee/issue_ai_entity.rb b/ee/app/serializers/ee/issue_ai_entity.rb index bf0010f23e9fd3..04af21d80a8e56 100644 --- a/ee/app/serializers/ee/issue_ai_entity.rb +++ b/ee/app/serializers/ee/issue_ai_entity.rb @@ -3,7 +3,7 @@ module EE class IssueAiEntity < ::IssueEntity expose :issue_comments do |_issue, options| - options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit]) + options[:resource].notes_with_limit(notes_limit: options[:notes_limit]) end end end diff --git a/ee/app/serializers/ee/merge_request/ai_entity.rb b/ee/app/serializers/ee/merge_request/ai_entity.rb index 751bfefecb36d9..5128320a28f6b9 100644 --- a/ee/app/serializers/ee/merge_request/ai_entity.rb +++ b/ee/app/serializers/ee/merge_request/ai_entity.rb @@ -4,7 +4,7 @@ module EE module MergeRequest class AiEntity < ::API::Entities::MergeRequest expose :mr_comments do |_mr, options| - options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit] / 2) + options[:resource].notes_with_limit(notes_limit: options[:notes_limit] / 2) end expose :diff do |mr, options| diff --git a/ee/app/serializers/epic_ai_entity.rb b/ee/app/serializers/epic_ai_entity.rb index 00dd6b482d96c3..0b4267c5c4d00f 100644 --- a/ee/app/serializers/epic_ai_entity.rb +++ b/ee/app/serializers/epic_ai_entity.rb @@ -3,7 +3,7 @@ # rubocop: disable Gitlab/NamespacedClass class EpicAiEntity < EpicEntity expose :epic_comments do |_epic, options| - options[:resource].notes_with_limit(options[:user], notes_limit: options[:notes_limit]) + options[:resource].notes_with_limit(notes_limit: options[:notes_limit]) end end # rubocop: enable Gitlab/NamespacedClass diff --git a/ee/lib/gitlab/llm/chain/gitlab_context.rb b/ee/lib/gitlab/llm/chain/gitlab_context.rb index 21e8cbd410dab4..a1e31aa6c309ce 100644 --- a/ee/lib/gitlab/llm/chain/gitlab_context.rb +++ b/ee/lib/gitlab/llm/chain/gitlab_context.rb @@ -23,20 +23,18 @@ def initialize( end def current_page_sentence - authorized_resource&.current_page_sentence(current_user: current_user) + authorized_resource&.current_page_sentence end def current_page_short_description - authorized_resource&.current_page_short_description(current_user: current_user) + authorized_resource&.current_page_short_description end def resource_serialized(content_limit:) return '' unless authorized_resource - authorized_resource.serialize_for_ai( - user: current_user, - content_limit: content_limit - ).to_xml(root: :root, skip_types: true, skip_instruct: true) + authorized_resource.serialize_for_ai(content_limit: content_limit) + .to_xml(root: :root, skip_types: true, skip_instruct: true) end private @@ -49,7 +47,7 @@ def authorized_resource return unless Utils::ChatAuthorizer.resource(resource: resource, user: current_user).allowed? - resource_wrapper_class.new(resource) + resource_wrapper_class.new(current_user, resource) end end end diff --git a/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb b/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb index 03bf24160199d2..f0aaa16295580e 100644 --- a/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/gitlab_context_spec.rb @@ -30,13 +30,13 @@ context 'with an authorized, serializable resource' do let(:resource) { create(:issue, project: project) } let(:resource_xml) do - Ai::AiResource::Issue.new(resource).serialize_for_ai(user: user, content_limit: content_limit) + Ai::AiResource::Issue.new(user, resource).serialize_for_ai(content_limit: content_limit) .to_xml(root: :root, skip_types: true, skip_instruct: true) end let(:resource2) { create(:issue, project: project) } let(:resource_xml2) do - Ai::AiResource::Issue.new(resource2).serialize_for_ai(user: user, content_limit: content_limit) + Ai::AiResource::Issue.new(user, resource2).serialize_for_ai(content_limit: content_limit) .to_xml(root: :root, skip_types: true, skip_instruct: true) end diff --git a/ee/spec/lib/gitlab/llm/chain/tools/epic_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/epic_reader/executor_spec.rb index 2a3366d8b6b20a..b899901278050a 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/epic_reader/executor_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/epic_reader/executor_spec.rb @@ -8,9 +8,8 @@ ai_request = double allow(ai_request).to receive(:request).and_return(ai_response) allow(context).to receive(:ai_request).and_return(ai_request) - resource_serialized = Ai::AiResource::Epic.new(resource) + resource_serialized = Ai::AiResource::Epic.new(context.current_user, resource) .serialize_for_ai( - user: context.current_user, content_limit: ::Gitlab::Llm::Chain::Tools::EpicReader::Prompts::Anthropic::MAX_CHARACTERS ).to_xml(root: :root, skip_types: true, skip_instruct: true) diff --git a/ee/spec/lib/gitlab/llm/chain/tools/issue_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/issue_reader/executor_spec.rb index 8657ba242051bb..c6616988fec378 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/issue_reader/executor_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/issue_reader/executor_spec.rb @@ -8,9 +8,8 @@ ai_request = double allow(ai_request).to receive(:request).and_return(ai_response) allow(context).to receive(:ai_request).and_return(ai_request) - resource_serialized = Ai::AiResource::Issue.new(resource) + resource_serialized = Ai::AiResource::Issue.new(context.current_user, resource) .serialize_for_ai( - user: context.current_user, content_limit: ::Gitlab::Llm::Chain::Tools::IssueReader::Prompts::Anthropic::MAX_CHARACTERS ).to_xml(root: :root, skip_types: true, skip_instruct: true) diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb index 09c467defff889..c0856c1b8fef18 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb @@ -8,9 +8,8 @@ ai_request = double allow(ai_request).to receive(:request).and_return(ai_response) allow(context).to receive(:ai_request).and_return(ai_request) - resource_serialized = Ai::AiResource::MergeRequest.new(resource) + resource_serialized = Ai::AiResource::MergeRequest.new(context.current_user, resource) .serialize_for_ai( - user: context.current_user, content_limit: ::Gitlab::Llm::Chain::Tools::MergeRequestReader::Prompts::Anthropic::MAX_CHARACTERS ).to_xml(root: :root, skip_types: true, skip_instruct: true) diff --git a/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb b/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb index 72be80aaea2b6f..5f1e9b81c6aae8 100644 --- a/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb +++ b/ee/spec/lib/gitlab/llm/completions/chat_real_requests_spec.rb @@ -89,7 +89,6 @@ where(:input_template, :tools) do 'Summarize this Merge Request' | %w[MergeRequestReader] 'Summarize %s Merge Request' | %w[MergeRequestReader] - 'Why did this pipeline fail?' | [] end with_them do diff --git a/ee/spec/models/ai/ai_resource/base_ai_resource_spec.rb b/ee/spec/models/ai/ai_resource/base_ai_resource_spec.rb index f1837751165a7f..c140ce3d0e620a 100644 --- a/ee/spec/models/ai/ai_resource/base_ai_resource_spec.rb +++ b/ee/spec/models/ai/ai_resource/base_ai_resource_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Ai::AiResource::BaseAiResource, feature_category: :duo_chat do describe '#serialize_for_ai' do it 'raises NotImplementedError' do - expect { described_class.new(nil).serialize_for_ai(_user: nil, _content_limit: nil) } + expect { described_class.new(nil, nil).serialize_for_ai(_content_limit: nil) } .to raise_error(NotImplementedError) end end diff --git a/ee/spec/models/ai/ai_resource/concerns/noteable_spec.rb b/ee/spec/models/ai/ai_resource/concerns/noteable_spec.rb index 624c00024fbacb..ca11070c3aca5b 100644 --- a/ee/spec/models/ai/ai_resource/concerns/noteable_spec.rb +++ b/ee/spec/models/ai/ai_resource/concerns/noteable_spec.rb @@ -8,7 +8,7 @@ let_it_be(:user) { create(:user) } let_it_be(:content_limit) { 1000 } - subject(:notes) { Ai::AiResource::Issue.new(issue).notes_with_limit(user, notes_limit: content_limit) } + subject(:notes) { Ai::AiResource::Issue.new(user, issue).notes_with_limit(notes_limit: content_limit) } context 'when user can see notes' do before do diff --git a/ee/spec/models/ai/ai_resource/epic_spec.rb b/ee/spec/models/ai/ai_resource/epic_spec.rb index 79e29df0956567..571c9de2b0e7be 100644 --- a/ee/spec/models/ai/ai_resource/epic_spec.rb +++ b/ee/spec/models/ai/ai_resource/epic_spec.rb @@ -6,7 +6,7 @@ let(:epic) { build(:epic) } let(:user) { build(:user) } - subject(:wrapped_epic) { described_class.new(epic) } + subject(:wrapped_epic) { described_class.new(user, epic) } describe '#serialize_for_ai' do it 'calls the serializations class' do @@ -19,7 +19,7 @@ resource: wrapped_epic }) - wrapped_epic.serialize_for_ai(user: user, content_limit: 100) + wrapped_epic.serialize_for_ai(content_limit: 100) end end diff --git a/ee/spec/models/ai/ai_resource/issue_spec.rb b/ee/spec/models/ai/ai_resource/issue_spec.rb index 5e3b6da73b0cb2..9f413c2ff605f0 100644 --- a/ee/spec/models/ai/ai_resource/issue_spec.rb +++ b/ee/spec/models/ai/ai_resource/issue_spec.rb @@ -6,7 +6,7 @@ let(:issue) { build(:issue) } let(:user) { build(:user) } - subject(:wrapped_issue) { described_class.new(issue) } + subject(:wrapped_issue) { described_class.new(user, issue) } describe '#serialize_for_ai' do it 'calls the serializations class' do @@ -18,7 +18,7 @@ serializer: 'ai', resource: wrapped_issue }) - wrapped_issue.serialize_for_ai(user: user, content_limit: 100) + wrapped_issue.serialize_for_ai(content_limit: 100) end end diff --git a/ee/spec/models/ai/ai_resource/merge_request_spec.rb b/ee/spec/models/ai/ai_resource/merge_request_spec.rb index 16eb29818bafff..b528a12a4e2a70 100644 --- a/ee/spec/models/ai/ai_resource/merge_request_spec.rb +++ b/ee/spec/models/ai/ai_resource/merge_request_spec.rb @@ -6,7 +6,7 @@ let(:merge_request) { build(:merge_request) } let(:user) { build(:user) } - subject(:wrapped_merge_request) { described_class.new(merge_request) } + subject(:wrapped_merge_request) { described_class.new(user, merge_request) } describe '#serialize_for_ai' do it 'calls the serializations class' do @@ -19,7 +19,7 @@ resource: wrapped_merge_request }) - wrapped_merge_request.serialize_for_ai(user: user, content_limit: 100) + wrapped_merge_request.serialize_for_ai(content_limit: 100) end end diff --git a/ee/spec/serializers/epic_serializer_spec.rb b/ee/spec/serializers/epic_serializer_spec.rb index 05985585a0fe77..09d74c8de57d40 100644 --- a/ee/spec/serializers/epic_serializer_spec.rb +++ b/ee/spec/serializers/epic_serializer_spec.rb @@ -28,7 +28,7 @@ context 'when ai serializer requested' do let(:json_entity) do described_class.new(current_user: user) - .represent(resource, serializer: 'ai', resource: Ai::AiResource::Epic.new(resource)) + .represent(resource, serializer: 'ai', resource: Ai::AiResource::Epic.new(user, resource)) .with_indifferent_access end diff --git a/ee/spec/serializers/issue_serializer_spec.rb b/ee/spec/serializers/issue_serializer_spec.rb index c61afeb8f2b0f3..aa8c26308eaa32 100644 --- a/ee/spec/serializers/issue_serializer_spec.rb +++ b/ee/spec/serializers/issue_serializer_spec.rb @@ -46,7 +46,7 @@ let(:json_entity) do described_class.new(current_user: user) .represent(resource, - serializer: serializer, resource: Ai::AiResource::Issue.new(resource)) + serializer: serializer, resource: Ai::AiResource::Issue.new(user, resource)) .with_indifferent_access end diff --git a/ee/spec/serializers/merge_request/ai_entity_spec.rb b/ee/spec/serializers/merge_request/ai_entity_spec.rb index 5e522107ddbefd..975674f6d03a46 100644 --- a/ee/spec/serializers/merge_request/ai_entity_spec.rb +++ b/ee/spec/serializers/merge_request/ai_entity_spec.rb @@ -10,7 +10,7 @@ let(:entity) do described_class.new(merge_request, user: user, - resource: Ai::AiResource::MergeRequest.new(merge_request), + resource: Ai::AiResource::MergeRequest.new(user, merge_request), notes_limit: notes_limit) end diff --git a/ee/spec/serializers/merge_request_serializer_spec.rb b/ee/spec/serializers/merge_request_serializer_spec.rb index 880fcc02a7c9ae..eb09ac5007ef46 100644 --- a/ee/spec/serializers/merge_request_serializer_spec.rb +++ b/ee/spec/serializers/merge_request_serializer_spec.rb @@ -12,7 +12,7 @@ .represent(merge_request, serializer: serializer, notes_limit: 2, - resource: Ai::AiResource::MergeRequest.new(merge_request)) + resource: Ai::AiResource::MergeRequest.new(user, merge_request)) .with_indifferent_access end -- GitLab From f69746bc24f7a3e1f05052bfb630b425193c6baa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 31 May 2024 19:06:37 +0200 Subject: [PATCH 26/32] Refactor MR Ai entity --- .../serializers/ee/merge_request/ai_entity.rb | 21 ------------------- .../serializers/ee/merge_request_ai_entity.rb | 19 +++++++++++++++++ .../ee/merge_request_serializer.rb | 2 +- ...pec.rb => merge_request_ai_entity_spec.rb} | 2 +- 4 files changed, 21 insertions(+), 23 deletions(-) delete mode 100644 ee/app/serializers/ee/merge_request/ai_entity.rb create mode 100644 ee/app/serializers/ee/merge_request_ai_entity.rb rename ee/spec/serializers/{merge_request/ai_entity_spec.rb => merge_request_ai_entity_spec.rb} (93%) diff --git a/ee/app/serializers/ee/merge_request/ai_entity.rb b/ee/app/serializers/ee/merge_request/ai_entity.rb deleted file mode 100644 index 5128320a28f6b9..00000000000000 --- a/ee/app/serializers/ee/merge_request/ai_entity.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -module EE - module MergeRequest - class AiEntity < ::API::Entities::MergeRequest - expose :mr_comments do |_mr, options| - options[:resource].notes_with_limit(notes_limit: options[:notes_limit] / 2) - end - - expose :diff do |mr, options| - ::Gitlab::Llm::Utils::MergeRequestTool.extract_diff( - source_project: mr.source_project, - source_branch: mr.source_branch, - target_project: mr.target_project, - target_branch: mr.target_branch, - character_limit: options[:notes_limit] / 2 - ) - end - end - end -end diff --git a/ee/app/serializers/ee/merge_request_ai_entity.rb b/ee/app/serializers/ee/merge_request_ai_entity.rb new file mode 100644 index 00000000000000..811915ec04565a --- /dev/null +++ b/ee/app/serializers/ee/merge_request_ai_entity.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module EE + class MergeRequestAiEntity < ::API::Entities::MergeRequest + expose :mr_comments do |_mr, options| + options[:resource].notes_with_limit(notes_limit: options[:notes_limit] / 2) + end + + expose :diff do |mr, options| + ::Gitlab::Llm::Utils::MergeRequestTool.extract_diff( + source_project: mr.source_project, + source_branch: mr.source_branch, + target_project: mr.target_project, + target_branch: mr.target_branch, + character_limit: options[:notes_limit] / 2 + ) + end + end +end diff --git a/ee/app/serializers/ee/merge_request_serializer.rb b/ee/app/serializers/ee/merge_request_serializer.rb index cf81735f75418f..761690e893e94b 100644 --- a/ee/app/serializers/ee/merge_request_serializer.rb +++ b/ee/app/serializers/ee/merge_request_serializer.rb @@ -7,7 +7,7 @@ module MergeRequestSerializer override :identified_entity def identified_entity(opts) if opts[:serializer] == 'ai' - MergeRequest::AiEntity + MergeRequestAiEntity else super(opts) end diff --git a/ee/spec/serializers/merge_request/ai_entity_spec.rb b/ee/spec/serializers/merge_request_ai_entity_spec.rb similarity index 93% rename from ee/spec/serializers/merge_request/ai_entity_spec.rb rename to ee/spec/serializers/merge_request_ai_entity_spec.rb index 975674f6d03a46..11973ea210e023 100644 --- a/ee/spec/serializers/merge_request/ai_entity_spec.rb +++ b/ee/spec/serializers/merge_request_ai_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MergeRequest::AiEntity, feature_category: :ai_abstraction_layer do +RSpec.describe EE::MergeRequestAiEntity, feature_category: :ai_abstraction_layer do # rubocop:disable RSpec/FilePath -- path is correct let_it_be(:user) { create(:user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it let_it_be(:merge_request) { create(:merge_request) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- we need create it let(:notes_limit) { 1000 } -- GitLab From c61a613603b8f5706301d4736e3000ce526fcb15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Sat, 1 Jun 2024 07:42:10 +0200 Subject: [PATCH 27/32] Refactor Model reference --- .../llm/chain/tools/merge_request_reader/prompts/anthropic.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb index c1e1002479e3b7..cd54204e8f5b6c 100644 --- a/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb +++ b/ee/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic.rb @@ -20,7 +20,7 @@ def self.prompt(options) { prompt: conversation, - options: { model: ::Gitlab::Llm::AiGateway::Client::CLAUDE_3_HAIKU } + options: { model: ::Gitlab::Llm::Anthropic::Client::CLAUDE_3_HAIKU } } end end -- GitLab From 8cd77899b4d90f9a2e1166c0841d2f3aedd43c56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Sat, 1 Jun 2024 08:40:19 +0200 Subject: [PATCH 28/32] Fix failing spec --- .../chain/tools/merge_request_reader/prompts/anthropic_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb index 709c08893af64a..f7e7d6520cfe0e 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/prompts/anthropic_spec.rb @@ -25,7 +25,7 @@ it "calls with haiku model" do model = described_class.prompt(options)[:options][:model] - expect(model).to eq(::Gitlab::Llm::AiGateway::Client::CLAUDE_3_HAIKU) + expect(model).to eq(::Gitlab::Llm::Anthropic::Client::CLAUDE_3_HAIKU) end end -- GitLab From f7a4a70621f79d75805e35ab703d59c77b05d9d1 Mon Sep 17 00:00:00 2001 From: lesley Date: Fri, 5 Jul 2024 14:36:46 -0500 Subject: [PATCH 29/32] Fix specs I --- .../agents/zero_shot/prompts/anthropic_spec.rb | 1 + .../tools/merge_request_reader/executor_spec.rb | 13 +++++++++---- ee/spec/lib/gitlab/llm/completions/chat_spec.rb | 11 +++++++---- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb b/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb index 0561a15b242634..8a0c0c9f560bdf 100644 --- a/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/agents/zero_shot/prompts/anthropic_spec.rb @@ -111,6 +111,7 @@ current_user: user, zero_shot_prompt: zero_shot_prompt, system_prompt: system_prompt, + unavailable_resources: '', source_template: "source template" } end diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb index c0856c1b8fef18..b750253c2e4d71 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb @@ -23,7 +23,9 @@ it 'returns success response' do allow(tool).to receive(:request).and_return(ai_response) - response = "I am sorry, I am unable to find what you are looking for." + response = "I'm sorry, I can't generate a response. You might want to try again. " \ + "You could also be getting this error because the items you're asking about " \ + "either don't exist, you don't have access to them, or your session has expired." expect(tool.execute.content).to eq(response) end end @@ -97,7 +99,9 @@ expect(tool).to receive(:request).exactly(3).times - response = "I am sorry, I am unable to find what you are looking for." + response = "I'm sorry, I can't generate a response. You might want to try again. " \ + "You could also be getting this error because the items you're asking about " \ + "either don't exist, you don't have access to them, or your session has expired." expect(tool.execute.content).to eq(response) end end @@ -109,7 +113,7 @@ allow(tool).to receive(:request).and_raise(StandardError) - expect(tool.execute.content).to eq("Unexpected error") + expect(tool.execute.content).to eq("I'm sorry, I can't generate a response. Please try again.") end end @@ -239,7 +243,8 @@ it 'returns success response' do allow(tool).to receive(:request).and_return(ai_response) - response = 'This feature is only allowed in groups or projects that enable this feature.' + response = "I am sorry, I cannot access the information you are asking about. " \ + "A group or project owner has turned off Duo features in this group or project." expect(tool.execute.content).to eq(response) end diff --git a/ee/spec/lib/gitlab/llm/completions/chat_spec.rb b/ee/spec/lib/gitlab/llm/completions/chat_spec.rb index 9501e44f2a74b2..9c80ad08c93d9c 100644 --- a/ee/spec/lib/gitlab/llm/completions/chat_spec.rb +++ b/ee/spec/lib/gitlab/llm/completions/chat_spec.rb @@ -230,10 +230,13 @@ ai_request: ai_request, extra_resource: extra_resource, request_id: 'uuid', current_file: current_file, agent_version: agent_version) .and_return(context) - expect(categorize_service).to receive(:execute) - expect(Llm::ExecuteMethodService).to receive(:new) - .with(user, user, :categorize_question, categorize_service_params) - .and_return(categorize_service) + # This is temporarily commented out due to the following production issue: + # https://gitlab.com/gitlab-com/gl-infra/production/-/issues/18191 + # Since the `#response_post_processing` call is commented out, this should be too. + # expect(categorize_service).to receive(:execute) + # expect(Llm::ExecuteMethodService).to receive(:new) + # .with(user, user, :categorize_question, categorize_service_params) + # .and_return(categorize_service) subject end -- GitLab From 3af7133b516f96700604707226895f4eef4297cd Mon Sep 17 00:00:00 2001 From: lesley Date: Tue, 16 Jul 2024 16:55:48 -0500 Subject: [PATCH 30/32] Fix specs II --- .../merge_request_reader/executor_spec.rb | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb index b750253c2e4d71..b4cc1ce3e72aad 100644 --- a/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/tools/merge_request_reader/executor_spec.rb @@ -235,16 +235,29 @@ context 'when group does not have ai enabled' do let(:identifier) { 'current' } let(:ai_response) { "current\", \"ResourceIdentifier\": \"#{identifier}\"}" } + let(:resource) { merge_request1 } before do stub_licensed_features(ai_chat: false) end - it 'returns success response' do - allow(tool).to receive(:request).and_return(ai_response) + it_behaves_like 'success response' + end - response = "I am sorry, I cannot access the information you are asking about. " \ + context 'when duo features are disabled for project' do + let(:identifier) { 'current' } + let(:ai_response) { "current\", \"ResourceIdentifier\": \"#{identifier}\"}" } + let(:response) do + "I am sorry, I cannot access the information you are asking about. " \ "A group or project owner has turned off Duo features in this group or project." + end + + before do + project.update!(duo_features_enabled: false) + end + + it 'returns success response' do + allow(tool).to receive(:request).and_return(ai_response) expect(tool.execute.content).to eq(response) end -- GitLab From 51dca95d80255b5a2d9763585b3287aac2831910 Mon Sep 17 00:00:00 2001 From: lesley Date: Fri, 19 Jul 2024 12:17:41 -0500 Subject: [PATCH 31/32] Add 'current_page_type' so v2_chat_agent_integration FF works --- ee/app/models/ai/ai_resource/merge_request.rb | 4 ++++ ee/spec/models/ai/ai_resource/merge_request_spec.rb | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/ee/app/models/ai/ai_resource/merge_request.rb b/ee/app/models/ai/ai_resource/merge_request.rb index 57f79215f7e848..077a4ff78b0b5c 100644 --- a/ee/app/models/ai/ai_resource/merge_request.rb +++ b/ee/app/models/ai/ai_resource/merge_request.rb @@ -15,6 +15,10 @@ def serialize_for_ai(content_limit:) }) end + def current_page_type + "merge_request" + end + def current_page_sentence return '' unless Feature.enabled?(:ai_merge_request_reader_for_chat, current_user) diff --git a/ee/spec/models/ai/ai_resource/merge_request_spec.rb b/ee/spec/models/ai/ai_resource/merge_request_spec.rb index b528a12a4e2a70..485e7e3a022c0c 100644 --- a/ee/spec/models/ai/ai_resource/merge_request_spec.rb +++ b/ee/spec/models/ai/ai_resource/merge_request_spec.rb @@ -23,6 +23,12 @@ end end + describe '#current_page_type' do + it 'returns type' do + expect(wrapped_merge_request.current_page_type).to eq('merge_request') + end + end + describe '#current_page_sentence' do it 'returns prompt' do expect(wrapped_merge_request.current_page_sentence) -- GitLab From ee4a078cf7b4c8b8ff01474cc230f3703651daf3 Mon Sep 17 00:00:00 2001 From: lesley Date: Fri, 19 Jul 2024 12:30:07 -0500 Subject: [PATCH 32/32] Update doc --- doc/user/gitlab_duo_chat/index.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/doc/user/gitlab_duo_chat/index.md b/doc/user/gitlab_duo_chat/index.md index 8c4384a5b29d60..41e315b08a0903 100644 --- a/doc/user/gitlab_duo_chat/index.md +++ b/doc/user/gitlab_duo_chat/index.md @@ -51,11 +51,12 @@ Other times, you must be more specific with your request. In the GitLab UI, GitLab Duo Chat knows about these areas: -| Area | How to ask Chat | -|---------|------------------| -| Epics | From the epic, ask about `this epic`, `this`, or the URL. From any UI area, ask about the URL. | -| Issues | From the issue, ask about `this issue`, `this`, or the URL. From any UI area, ask about the URL. | -| Code files | From the single file, ask about `this code` or `this file`. From any UI area, ask about the URL. | +| Area | How to ask Chat | +|---------------|------------------------------------------------------------------------------------------------------------------| +| Epics | From the epic, ask about `this epic`, `this`, or the URL. From any UI area, ask about the URL. | +| Issues | From the issue, ask about `this issue`, `this`, or the URL. From any UI area, ask about the URL. | +| Merge Request | From the merge request, ask about `this merge request`, `this`, or the URL. From any UI area, ask about the URL. | +| Code files | From the single file, ask about `this code` or `this file`. From any UI area, ask about the URL. | In the IDEs, GitLab Duo Chat knows about these areas: -- GitLab