Skip to content
Snippets Groups Projects
Verified Commit 620725c2 authored by Sincheol (David) Kim's avatar Sincheol (David) Kim :two: Committed by GitLab
Browse files

Merge branch 'ck3g-remove-duo_code_review_multi_file-feature-flag' into 'master' 

Remove duo_code_review_multi_file feature flag

See merge request gitlab-org/gitlab!192762



Merged-by: default avatarSincheol (David) Kim <dkim@gitlab.com>
Approved-by: default avatarKinshuk Singh <kinsingh@gitlab.com>
Approved-by: default avatarSincheol (David) Kim <dkim@gitlab.com>
Reviewed-by: default avatarKinshuk Singh <kinsingh@gitlab.com>
Co-authored-by: default avatarVitali Tatarintev <vtatarintev@gitlab.com>
parents 9244e5a1 76fdd56e
No related branches found
No related tags found
No related merge requests found
Show whitespace changes
Inline Side-by-side
Showing
with 61 additions and 310 deletions
config/feature_flags/beta/duo_code_review_multi_file.yml deleted 100644 → 0
+ 0
− 9
View file @ 9244e5a1
---
name: duo_code_review_multi_file
feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/532653
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188117
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/536005
milestone: '18.0'
group: group::code review
type: beta
default_enabled: true
ee/lib/gitlab/llm/anthropic/completions/review_merge_request.rb
+ 2
− 29
View file @ 620725c2
......@@ -102,11 +102,7 @@ def perform_review
mr_diff_refs = merge_request.diff_refs
if Feature.enabled?(:duo_code_review_multi_file, user)
process_all_files_together(diff_files, mr_diff_refs)
else
process_files_individually(diff_files, mr_diff_refs)
end
process_files(diff_files, mr_diff_refs)
if @draft_notes_by_priority.empty?
update_progress_note(review_summary, with_todo: true)
......@@ -119,7 +115,7 @@ def perform_review
log_comment_metrics
end
def process_all_files_together(diff_files, mr_diff_refs)
def process_files(diff_files, mr_diff_refs)
diffs_and_paths = {}
files_content = {}
......@@ -156,29 +152,6 @@ def process_all_files_together(diff_files, mr_diff_refs)
end
end
def process_files_individually(diff_files, mr_diff_refs)
diff_files.each do |diff_file|
single_file_diff = { diff_file.new_path => diff_file.raw_diff }
single_file_content = {}
if !diff_file.new_file? && include_file_content?
content = diff_file.old_blob&.data
single_file_content[diff_file.new_path] = content if content.present?
end
review_prompt = generate_review_prompt(single_file_diff, single_file_content)
next unless review_prompt.present?
response = review_response_for(review_prompt)
next if note_not_required?(response)
parsed_body = ResponseBodyParser.new(response.response_body)
file_comments = parsed_body.comments.select { |comment| comment.file == diff_file.new_path }
process_comments(file_comments, diff_file, mr_diff_refs)
end
end
def process_review_with_retry(diffs_and_paths, files_content)
# First try with file content (if any)
if files_content.present?
......
ee/lib/gitlab/llm/templates/review_merge_request.rb
+ 8
− 110
View file @ 620725c2
......@@ -16,90 +16,6 @@ class ReviewMergeRequest
PROMPT
)
SINGLE_FILE_MESSAGE = Gitlab::Llm::Chain::Utils::Prompt.as_user(
<<~PROMPT.chomp
First, you will be given the merge request title and description to understand the purpose of these changes, followed by a filename and a structured representation of the git diff for that file. This structured diff contains the changes made in the MR that you need to review.%{full_file_intro}
Merge Request Title:
<mr_title>
%{mr_title}
</mr_title>
Merge Request Description:
<mr_description>
%{mr_description}
</mr_description>
Here is the filename of the git diff:
<filename>
%{new_path}
</filename>
Here is the git diff you need to review:
<git_diff>
%{diff_lines}
</git_diff>
%{full_content_section}
To properly review this MR, follow these steps:
1. Parse the git diff:
- Each `<line>` tag inside of the `<git_diff>` tag represents a line in git diff
- The `type` attribute in `<line>` tag specifies whether the line is "context" (unchanged), "added", or "deleted"
- `old_line` attribute in `<line>` tag represents the old line number before the change
- `old_line` will be empty if the line is a newly added line
- `new_line` attribute in `<line>` tag represents the new line number after the change
- `new_line` will be empty if the line is a deleted line
- Context (unchanged) lines will have both `old_line` and `new_line`, but the line number may have changed if any changes were made above the line
- `<chunk_header>` tags may be present to indicate the location of changes in the file (e.g., "@@ -13,6 +16,7 @@")
2. Analyze the changes carefully, strictly focus on the following criteria:
- Code correctness and functionality
- Code efficiency and performance impact
- Potential security vulnerabilities like SQL injection, XSS, etc.
- Potential bugs or edge cases that may have been missed
- Do not comment on documentations
3. Formulate your comments:
- Determine the most appropriate line for your comment
- When you notice multiple issues on the same line, leave only one comment on that line and list your issues together. List comments from highest in priority to the lowest.
- Assign each comment a priority from 1 to 3:
- Priority 1: Not important
- Priority 2: Helpful but can be ignored
- Priority 3: Important, helpful and required
4. Format your comments:
- Wrap each comment in a <comment> element
- Include a `file` attribute with the filename "%{new_path}"
- Include a `priority` attribute with the assigned priority (1, 2, or 3)
- Include the `old_line` and `new_line` attributes exactly as they appear in the chosen `<line>` tag for the comment
- When suggesting a change, use the following format:
<from>
[existing lines that you are suggesting to change]
</from>
<to>
[your suggestion]
</to>
- <from> tag must be identical to the lines as they appear in the diff, including any leading spaces or tabs
- <to> tag must contain your suggestion
- Opening and closing `<from>` and `<to>` tags should not be on the same line as the content
- When making suggestions, always maintain the exact indentation as shown in the original diff. The suggestion should match the indentation of the line you are commenting on precisely, as it will be applied directly in place of the existing line.
- Your suggestion must only include the lines that are actually changing from the existing lines
- Do not include any code suggestions when you are commenting on a deleted line since suggestions cannot be applied on deleted lines
- Wrap your entire response in `<review></review>` tag.
- Just return `<review></review>` as your entire response, if the change is acceptable
Pay careful attention to the Merge Request title and description to understand the purpose of the changes. Some changes may involve intentional removals or modifications that align with the MR's stated goals. Note that in some cases, the MR description may not be provided.
Remember to only focus on substantive feedback that will genuinely improve the code or prevent potential issues. Do not nitpick or comment on trivial matters.
Begin your review now.
PROMPT
)
MULTI_FILE_MESSAGE = Gitlab::Llm::Chain::Utils::Prompt.as_user(
<<~PROMPT.chomp
First, you will be given the merge request title and description to understand the purpose of these changes, followed by a structured representation of the git diffs for all changed files in this merge request. These structured diffs contain the changes that you need to review.%{full_file_intro}
......@@ -200,7 +116,6 @@ def to_prompt
end
def to_prompt_inputs
# Only include variables for multi file since that should be a default soon
variables.slice(:mr_title, :mr_description, :diff_lines, :full_file_intro, :full_content_section)
end
......@@ -210,40 +125,23 @@ def model_version
private
def multi_file?
Feature.enabled?(:duo_code_review_multi_file, user)
end
strong_memoize_attr :multi_file?
def full_file?
Feature.enabled?(:duo_code_review_full_file, user)
end
strong_memoize_attr :full_file?
def user_message
multi_file? ? MULTI_FILE_MESSAGE : SINGLE_FILE_MESSAGE
MULTI_FILE_MESSAGE
end
def variables
template_variables = if multi_file?
{
template_variables = {
mr_title: mr_title,
mr_description: mr_description,
diff_lines: all_diffs_formatted,
full_file_intro: "",
full_content_section: ""
}
else
path, raw_diff = diffs_and_paths.first
{
mr_title: mr_title,
mr_description: mr_description,
new_path: path,
diff_lines: format_diff(raw_diff),
full_file_intro: "",
full_content_section: ""
}
end
# Add full file content if the feature flag is enabled
add_full_file_content(template_variables) if full_file? && files_content.present?
......
ee/spec/lib/gitlab/llm/anthropic/completions/review_merge_request_spec.rb
+ 0
− 52
View file @ 620725c2
......@@ -671,58 +671,6 @@
completion.execute
end
context 'with duo_code_review_multi_file disabled and multiple files' do
let(:client) { instance_double(Gitlab::Llm::Anthropic::Client) }
let(:first_file_prompt) { { messages: ['First file prompt'] } }
let(:second_file_prompt) { { messages: ['Second file prompt'] } }
let(:first_response) do
{ content: [{ text: '<review><comment file="UPDATED.md" priority="3" old_line="" new_line="2">' \
'First file comment</comment></review>' }] }
end
let(:second_response) do
{ content: [{ text: '<review><comment file="NEW.md" priority="3" old_line="" new_line="1">' \
'Second file comment</comment></review>' }] }
end
before do
stub_feature_flags(duo_code_review_multi_file: false)
# Set up client and responses
allow(Gitlab::Llm::Anthropic::Client).to receive(:new).and_return(client)
# Set up individual file prompts
allow_next_instance_of(review_prompt_class) do |template|
path = template.instance_variable_get(:@diffs_and_paths)&.keys&.first
case path
when 'UPDATED.md'
allow(template).to receive(:to_prompt).and_return(first_file_prompt)
allow(client).to receive(:messages_complete).with(first_file_prompt).and_return(first_response)
when 'NEW.md'
allow(template).to receive(:to_prompt).and_return(second_file_prompt)
allow(client).to receive(:messages_complete).with(second_file_prompt).and_return(second_response)
end
end
allow(client).to receive(:messages_complete).with(summary_prompt).and_return(summary_response&.to_json)
end
it 'makes separate requests for each file and creates notes' do
# Set expectations for separate requests
expect(client).to receive(:messages_complete).with(first_file_prompt).once
expect(client).to receive(:messages_complete).with(second_file_prompt).once
completion.execute
# Verify notes were created for both files
diff_notes = merge_request.notes.diff_notes.authored_by(duo_code_review_bot).reorder(:id)
expect(diff_notes.count).to eq 2
expect(diff_notes.pluck(:note)).to match_array(['First file comment', 'Second file comment'])
expect(diff_notes.map { |n| n.position.new_path }).to contain_exactly('UPDATED.md', 'NEW.md')
end
end
context 'when logging LLM response metrics' do
before do
# Ignore other logs
......
ee/spec/lib/gitlab/llm/templates/review_merge_request_spec.rb
+ 51
− 110
View file @ 620725c2
......@@ -44,16 +44,23 @@
).to_prompt
end
subject(:user_prompt) { prompt&.dig(:messages, 0, :content) }
before do
stub_feature_flags(duo_code_review_multi_file: false)
let(:diffs_and_paths) do
{
'UPDATED.md' =>
"@@ -1,4 +1,4 @@\n # UPDATED\n-Welcome\n-This is an updated file\n+Welcome!\n+This is an updated file.",
'OTHER.md' => "@@ -5,3 +5,3 @@\n # CONTENT\n-This is old content\n+This is updated content"
}
end
it 'includes new_path' do
expect(user_prompt).to include("<filename>\n#{new_path}\n</filename>")
let(:files_content) do
{
'UPDATED.md' => "# UPDATED\nWelcome!\nThis is an updated file.\n\n...",
'OTHER.md' => "Some header\n\n...\n\n# CONTENT\n\nThis is updated content"
}
end
subject(:user_prompt) { prompt&.dig(:messages, 0, :content) }
it 'includes merge request title' do
expect(user_prompt).to include('Fix typos in welcome message')
end
......@@ -62,51 +69,6 @@
expect(user_prompt).to include('Improving readability by fixing typos and adding proper punctuation.')
end
it 'includes diff lines with hunk header' do
expect(user_prompt).to include(
<<~CONTENT
<line type="context" old_line="1" new_line="1"># UPDATED</line>
<line type="context" old_line="2" new_line="2"></line>
<line type="deleted" old_line="3" new_line="">Welcome</line>
<line type="deleted" old_line="4" new_line="">This is an updated file</line>
<line type="added" old_line="" new_line="3">Welcome!</line>
<line type="added" old_line="" new_line="4">This is an updated file.</line>
<chunk_header>@@ -10,3 +10,3 @@</chunk_header>
<line type="context" old_line="10" new_line="10"># ANOTHER HUNK</line>
<line type="context" old_line="11" new_line="11"></line>
<line type="deleted" old_line="12" new_line="">This is an old line</line>
<line type="added" old_line="" new_line="12">This is a new line</line>
CONTENT
)
end
context 'when duo_code_review_full_file feature flag is enabled' do
it 'includes the original file content introduction text' do
expect(user_prompt).to include("You will also be provided with the original content of modified files " \
"(before changes). Newly added files are not included as their full content is already in the diffs.")
end
it 'includes the original file content' do
expect(user_prompt).to include("<full_file filename=\"#{new_path}\">")
expect(user_prompt).to include(files_content['UPDATED.md'])
end
end
context 'when duo_code_review_full_file feature flag is disabled' do
before do
stub_feature_flags(duo_code_review_full_file: false)
end
it 'does not include the full file content introduction text' do
expect(user_prompt).not_to include("You will also be provided with the original content of modified files " \
"(before changes).")
end
it 'does not include the full file content' do
expect(user_prompt).not_to include("<full_file_content>\n#{files_content['UPDATED.md']}\n</full_file_content>")
end
end
it 'uses Claude 3.7 Sonnet' do
expect(prompt[:model]).to eq(::Gitlab::Llm::Anthropic::Client::CLAUDE_3_7_SONNET)
end
......@@ -123,26 +85,6 @@
expect(prompt[:timeout]).to eq(described_class::TIMEOUT)
end
context 'with multiple files' do
before do
stub_feature_flags(duo_code_review_multi_file: true)
end
let(:diffs_and_paths) do
{
'UPDATED.md' =>
"@@ -1,4 +1,4 @@\n # UPDATED\n-Welcome\n-This is an updated file\n+Welcome!\n+This is an updated file.",
'OTHER.md' => "@@ -5,3 +5,3 @@\n # CONTENT\n-This is old content\n+This is updated content"
}
end
let(:files_content) do
{
'UPDATED.md' => "# UPDATED\nWelcome!\nThis is an updated file.\n\n...",
'OTHER.md' => "Some header\n\n...\n\n# CONTENT\n\nThis is updated content"
}
end
it 'includes both file diffs' do
expect(user_prompt).to include('<file_diff filename="UPDATED.md">')
expect(user_prompt).to include('<file_diff filename="OTHER.md">')
......@@ -195,7 +137,6 @@
end
end
end
end
describe '#to_prompt_inputs' do
let(:expected_diff_lines) do
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment