Attach resolution note at vulnerability's detected line

Context
-----------

We have a feature, ["Resolve vulnerability"][0], that currently
generates a merge request to resolve a vulnerability. The only
existing ingress into this workflow Is through the vulnerability
detail page.

We are building out a new ingress to the "Resolve vulnerability"
workflow from the security widget on an existing MR.

When the resolution MR is generated from a vulnerable MR's security
widget, we attach a note to the vulnerable MR that provides a link to
the resolution MR.[^1]

This change
-----------

Right now, the note is just attached on the MR without being tied to a
specific line. We want to attach the note at the line the
vulnerability finding is being detected.

There are complications[^2] with attaching a `DiffNote` to a line that
isn't a part of the MR's diff. It is described more in this
[issue][2].

For this initial implementation, we just fall back to using an
un-attached note for situations where the finding's detected line is
not in the diff.

[0]:https://docs.gitlab.com/ee/user/application_security/vulnerabilities/#resolve-a-vulnerability-with-a-merge-request
[1]:https://gitlab.com/gitlab-org/gitlab/-/merge_requests/172185
[2]:https://gitlab.com/gitlab-org/gitlab/-/issues/325161
[3]:https://gitlab.com/gitlab-org/gitlab/-/merge_requests/173238#note_2223845037

---

MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/173238
Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/503403
Changelog: changed
EE: true

[^1]: Originally, this new workflow was implemented using code change
      suggestions on the vulnerable MR. There was no secondary resolution
      MR created. We had to pivot the workflow to use a separate resolution
      MR. This change is a follow-up refinement from that pivot MR. To get
      more context on why we had to pivot you can see that MR [here][1]

[^2]: There is a lot more context in this [MR thread][3]

ee/app/services/vulnerabilities/remediations/note_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  module Remediations
+    class NoteService
+      include Gitlab::Utils::StrongMemoize
+
+      RESOLUTION_NOTE_TEXT = 'Vulnerability Resolution has generated a fix in an MR for this vulnerability.' \
+        '%{line_break}%{line_break}In order to apply the fix, merge the following MR:' \
+        '%{line_break}%{resolution_mr_url}+s'
+
+      def initialize(vulnerable_mr, resolution_mr, vulnerability, user)
+        raise_argument_error if nil_params?(vulnerable_mr, resolution_mr, vulnerability, user)
+
+        @vulnerable_mr = vulnerable_mr
+        @project = vulnerable_mr.project
+        @user = user
+        # rubocop:disable CodeReuse/Presenter -- Presenter methods are used in ERB presentation.
+        @finding_presenter = Vulnerabilities::FindingPresenter.new(vulnerability.finding)
+        # rubocop:enable CodeReuse/Presenter
+        @resolution_mr_url = Gitlab::Routing.url_helpers.project_merge_request_url(
+          project,
+          resolution_mr
+        )
+      end
+
+      def execute
+        note = Notes::CreateService.new(project, user, attrs.compact).execute
+
+        note.valid? ? note : nil
+      end
+
+      private
+
+      attr_reader :vulnerable_mr, :resolution_mr_url, :user, :project, :finding_presenter
+
+      def raise_argument_error
+        raise ArgumentError, 'All params required to be non-nil'
+      end
+
+      def nil_params?(*params)
+        params.any?(&:nil?)
+      end
+
+      def note_text
+        format(_(RESOLUTION_NOTE_TEXT), resolution_mr_url: resolution_mr_url, line_break: '<br/>')
+      end
+
+      def attrs
+        attrs = {
+          position: {
+            position_type: "text",
+            new_path: file,
+            base_sha: vulnerable_mr.latest_merge_request_diff&.base_commit_sha,
+            head_sha: vulnerable_mr.latest_merge_request_diff&.head_commit_sha,
+            start_sha: vulnerable_mr.latest_merge_request_diff&.start_commit_sha,
+            ignore_whitespace_change: false,
+            new_line: start_line
+          },
+          note: note_text,
+          type: "DiffNote",
+          noteable_type: 'MergeRequest',
+          noteable_id: vulnerable_mr.id
+        }
+
+        # look for a line in the diff where the new line number
+        # matches the finding line number. If we find this, we
+        # will attach the note at the line we found
+        vulnerable_mr.raw_diffs(limits: true, paths: [file]).each do |diff|
+          next unless diff.new_path == file
+
+          lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
+          line = lines.find { |line| line.new_pos == start_line }
+
+          attrs[:line_code] = Gitlab::Git.diff_line_code(diff.new_path, line.new_pos, line.old_pos) if line
+        end
+
+        # It is possible the finding was detected in the file, but
+        # the line it was detected on is an unchanged line and is
+        # not in the diff.
+        #
+        # In this case, we change the note to be a general
+        # discussion note and it isn't attached to a specific
+        # line.
+        if attrs[:line_code].nil?
+          attrs[:type] = "DiscussionNote"
+          attrs[:position] = nil
+        end
+
+        attrs
+      end
+
+      def file
+        finding_presenter.file
+      end
+      strong_memoize_attr :file
+
+      def start_line
+        finding_presenter.location[:start_line]
+      end
+      strong_memoize_attr :start_line
+    end
+  end
+end

ee/lib/gitlab/llm/completions/resolve_vulnerability/helpers.rb

@@ -22,9 +22,6 @@ module Helpers
           END_SUMMARY = "</summary>"
           START_IS_FALSE_POSITIVE = "<is_false_positive>\n"
           END_IS_FALSE_POSITIVE = "</is_false_positive>"
-          RESOLUTION_NOTE_TEXT = 'Vulnerability Resolution has generated a fix in an MR for this vulnerability.' \
-            '%{line_break}%{line_break}In order to apply the fix, merge the following MR:' \
-            '%{line_break}%{resolution_mr_url}+s'
 
           private
 
@@ -132,7 +129,11 @@ def create_merge_request_with_resolution(
               merge_request
             )
 
-            note = add_note_from_vulnerable_mr_to_resolution(vulnerable_mr, merge_request, vulnerability, user)
+            if vulnerable_mr
+              note = Vulnerabilities::Remediations::NoteService.new(
+                vulnerable_mr, merge_request, vulnerability, user).execute
+            end
+
             note_url = Gitlab::UrlBuilder.note_url(note) if note
 
             {
@@ -141,39 +142,6 @@ def create_merge_request_with_resolution(
             }
           end
 
-          def add_note_from_vulnerable_mr_to_resolution(vulnerable_mr, resolution_mr, vulnerability, user)
-            return unless vulnerable_mr
-
-            finding_presenter = Vulnerabilities::FindingPresenter.new(vulnerability.finding)
-            resolution_mr_url = Gitlab::Routing.url_helpers.project_merge_request_url(
-              resolution_mr.project,
-              resolution_mr
-            )
-            formatted_note = format(_(RESOLUTION_NOTE_TEXT), resolution_mr_url: resolution_mr_url, line_break: '<br/>')
-
-            attrs = {
-              position: {
-                position_type: "text",
-                old_path: finding_presenter.file,
-                new_path: finding_presenter.file,
-                base_sha: vulnerable_mr.latest_merge_request_diff.base_commit_sha,
-                head_sha: vulnerable_mr.latest_merge_request_diff.head_commit_sha,
-                start_sha: vulnerable_mr.latest_merge_request_diff.start_commit_sha,
-                ignore_whitespace_change: false,
-                new_line: finding_presenter.location[:start_line],
-                old_line: finding_presenter.location[:start_line]
-              },
-              note: formatted_note,
-              type: "DiscussionNote",
-              noteable_type: MergeRequest,
-              noteable_id: vulnerable_mr.id
-            }
-
-            note = Notes::CreateService.new(vulnerable_mr.project, user, attrs).execute
-
-            note.valid? ? note : nil
-          end
-
           def merge_request_gid(merge_request_id)
             return unless merge_request_id
 

ee/spec/lib/gitlab/llm/completions/resolve_vulnerability_spec.rb

@@ -367,26 +367,21 @@ def expect_logged_error(error)
 
         let(:options) { { vulnerable_merge_request_id: mr_global_id } }
         let(:mr_global_id) { Gitlab::GlobalId.as_global_id(vulnerable_mr.id, model_name: MergeRequest) }
+        let(:note_service) { instance_double(Vulnerabilities::Remediations::NoteService) }
         let(:note) { create(:note_on_merge_request, project: vulnerable_mr.project, noteable: vulnerable_mr) }
-        let(:expected_params) do
-          note_regex = %r{Vulnerability Resolution has generated a fix.*merge_requests.*/\d+\+s.*}
-          [
-            vulnerable_mr.project,
-            user,
-            a_hash_including(note: a_string_matching(note_regex), noteable_id: vulnerable_mr.id)
-          ]
-        end
 
         before do
-          allow(Notes::CreateService).to receive(:new).and_call_original
-          allow_next_instance_of(Notes::CreateService) do |note_service|
-            allow(note_service).to receive(:execute).and_return(note)
-          end
+          allow(Vulnerabilities::Remediations::NoteService).to receive(:new).and_return(note_service)
+          allow(note_service).to receive(:execute).and_return(note)
         end
 
         it 'creates a note on the vulnerable merge request' do
           resolve.execute
-          expect(Notes::CreateService).to have_received(:new).with(*expected_params)
+
+          expected_params = [vulnerable_mr, merge_request, vulnerability, user]
+
+          expect(Vulnerabilities::Remediations::NoteService).to have_received(:new).with(*expected_params)
+          expect(note_service).to have_received(:execute).exactly(:once)
         end
 
         it 'tracks internal event with success' do

ee/spec/services/vulnerabilities/remediations/note_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::Remediations::NoteService, feature_category: :vulnerability_management do
+  let_it_be(:user) { create(:user) }
+  let_it_be_with_reload(:vulnerable_mr) { create(:merge_request) }
+  let_it_be(:project) { vulnerable_mr.project }
+  let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) }
+  let_it_be(:resolution_mr) do
+    create(
+      :merge_request, :simple,
+      source_branch: 'resolution', target_branch: vulnerable_mr.source_branch, source_project: project
+    )
+  end
+
+  let(:service) { described_class.new(*params) }
+  let(:params) { [vulnerable_mr, resolution_mr, vulnerability, user] }
+
+  describe '#execute' do
+    subject(:response) { described_class.new(*params).execute }
+
+    let(:note_regex) { %r{Vulnerability Resolution has generated a fix.*merge_requests.*/\d+\+s.*} }
+
+    context 'when a line_code can be generated' do
+      let_it_be(:file) { vulnerable_mr.raw_diffs.first.new_path }
+      let_it_be(:finding_location) { { start_line: 1 } }
+
+      before do
+        allow_next_instance_of(Vulnerabilities::FindingPresenter) do |presenter|
+          allow(presenter).to receive_messages(file: file, location: finding_location)
+        end
+      end
+
+      it 'creates a diff note' do
+        expect(response.valid?).to be true
+        expect(response.noteable_type).to eq('MergeRequest')
+        expect(response.noteable_id).to eq(vulnerable_mr.id)
+        expect(response.type).to eq('DiffNote')
+        expect(response.note).to match(note_regex)
+        expect(response.line_code).to eq(Gitlab::Git.diff_line_code(file, 1, 0))
+      end
+    end
+
+    context 'when a line_code cannot be generated' do
+      it 'creates a discussion note' do
+        expect(response.valid?).to be true
+        expect(response.noteable_type).to eq('MergeRequest')
+        expect(response.noteable_id).to eq(vulnerable_mr.id)
+        expect(response.type).to eq('DiscussionNote')
+        expect(response.note).to match(note_regex)
+      end
+    end
+  end
+
+  context 'with missing parameters' do
+    it 'raises an argument error' do
+      params.each_with_index do |_, i|
+        test_params = params.dup.tap { |p| p[i] = nil }
+
+        expect { described_class.new(*test_params) }.to raise_error(ArgumentError)
+      end
+    end
+  end
+end