diff --git a/danger/internal_users/Dangerfile b/danger/internal_users/Dangerfile new file mode 100644 index 0000000000000000000000000000000000000000..c86353394008b912de08e4fbb7c91811abd56120 --- /dev/null +++ b/danger/internal_users/Dangerfile @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +internal_users.add_comment_for_internal_users_changes diff --git a/danger/plugins/internal_users.rb b/danger/plugins/internal_users.rb new file mode 100644 index 0000000000000000000000000000000000000000..5506b026c5cbca7deea35d5862a4a68a85613e15 --- /dev/null +++ b/danger/plugins/internal_users.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require_relative '../../tooling/danger/internal_users' + +module Danger + class InternalUsers < Plugin + include Tooling::Danger::InternalUsers + end +end diff --git a/spec/tooling/danger/internal_users_spec.rb b/spec/tooling/danger/internal_users_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..90472ae9a4818faf13a79fdbf9aa7324770ad9d0 --- /dev/null +++ b/spec/tooling/danger/internal_users_spec.rb @@ -0,0 +1,263 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'gitlab/dangerfiles/spec_helper' + +require_relative '../../../tooling/danger/internal_users' +require_relative '../../../tooling/danger/project_helper' + +RSpec.describe Tooling::Danger::InternalUsers, feature_category: :tooling do + include_context "with dangerfile" + + let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) } + let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) } + let(:file_path) { 'app/models/user.rb' } + let(:changed_lines) { [] } + let(:file_lines) { [] } + + subject(:helper) { fake_danger.new(helper: fake_helper) } + + before do + allow(helper).to receive(:project_helper).and_return(fake_project_helper) + allow(fake_helper).to receive(:all_changed_files).and_return([file_path]) + allow(fake_helper).to receive(:changed_lines).with(file_path).and_return(changed_lines) + allow(fake_project_helper).to receive(:file_lines).with(file_path).and_return(file_lines) + end + + describe '#add_comment_for_internal_users_changes' do + shared_examples 'no violations' do + it 'adds no comments' do + expect(helper).not_to receive(:markdown) + expect(helper).not_to receive(:warn) + + helper.add_comment_for_internal_users_changes + end + end + + shared_examples 'has violations' do + it 'adds file and MR level comments' do + expect(helper).to receive(:markdown).once + expect(helper).to receive(:warn).once + + helper.add_comment_for_internal_users_changes + end + end + + context 'when documentation is changed' do + before do + allow(fake_helper).to receive(:all_changed_files).and_return([described_class::DOCS_PATH]) + end + + include_examples 'no violations' + end + + context 'when method name is changed' do + let(:changed_lines) do + [ + '- def security_bot', + '+ def security_bot_v2' + ] + end + + let(:file_lines) { [' def security_bot_v2'] } + + include_examples 'has violations' + end + + context 'when method body is changed' do + let(:changed_lines) do + [ + ' def security_bot', + '+ super', + ' end' + ] + end + + let(:file_lines) do + [ + 'def security_bot', + ' super', + 'end' + ] + end + + include_examples 'has violations' + end + + context 'when bot module is referenced' do + let(:changed_lines) do + [ + '+ Users::Internal.security_bot' + ] + end + + let(:file_lines) { [' Users::Internal.security_bot'] } + + include_examples 'has violations' + end + + context 'when bot symbol is used' do + let(:changed_lines) do + [ + '+ user_type: :security_bot' + ] + end + + let(:file_lines) { [' user_type: :security_bot'] } + + include_examples 'has violations' + end + + context 'when changes are unrelated' do + let(:changed_lines) do + [ + '+ user.name = "Regular User"' + ] + end + + let(:file_lines) { [' user.name = "Regular User"'] } + + include_examples 'no violations' + end + + context 'when changing lines inside a bot method definition' do + let(:changed_lines) do + [ + ' user.name = "Bot Name"' + ] + end + + let(:file_lines) do + [ + 'def security_bot', + ' user.name = "Bot Name"', + 'end' + ] + end + + include_examples 'has violations' + end + + context 'when changing lines outside any bot method definition' do + let(:changed_lines) do + [ + ' user.name = "Regular Name"' + ] + end + + let(:file_lines) do + [ + 'def regular_method', + ' user.name = "Regular Name"', + 'end' + ] + end + + include_examples 'no violations' + end + + context 'when bot method spans multiple ends' do + let(:changed_lines) do + [ + ' user.tap do |u|', + ' u.name = "Bot Name"', + ' end' + ] + end + + let(:file_lines) do + [ + 'def security_bot', + ' user.tap do |u|', + ' u.name = "Bot Name"', + ' end', + 'end' + ] + end + + include_examples 'has violations' + end + + context 'when modifying code between bot methods' do + let(:changed_lines) do + [ + ' user.name = "Regular Name"' + ] + end + + let(:file_lines) do + [ + 'def security_bot', + ' # bot code', + 'end', + ' user.name = "Regular Name"', + 'def alert_bot', + ' # bot code', + 'end' + ] + end + + include_examples 'no violations' + end + + context 'when file is not a ruby file' do + let(:file_path) { 'some_file.png' } + let(:changed_lines) { [] } + + include_examples 'no violations' + end + end + + describe '#file_has_violations?' do + context 'when tracking bot method boundaries' do + let(:changed_lines) { [' some_code'] } + + it 'correctly tracks entering and exiting bot methods' do + allow(fake_project_helper).to receive(:file_lines).and_return([ + 'def security_bot', + ' some_code', + 'end' + ]) + + expect(helper.send(:file_has_violations?, file_path)).to be true + + allow(fake_project_helper).to receive(:file_lines).and_return([ + 'def security_bot', + ' other_code', + 'end', + ' some_code' + ]) + + expect(helper.send(:file_has_violations?, file_path)).to be false + end + + it 'handles nested end keywords' do + allow(fake_project_helper).to receive(:file_lines).and_return([ + 'def security_bot', + ' some_code', + ' if true', + ' some_code', + ' end', + 'end' + ]) + + expect(helper.send(:file_has_violations?, file_path)).to be true + end + + it 'processes all lines in method body' do + lines = [ + 'def security_bot', + ' line1', + ' line2', + ' line3', + 'end' + ] + + allow(fake_project_helper).to receive(:file_lines).and_return(lines) + allow(fake_helper).to receive(:changed_lines).and_return([' line2']) + + expect(helper.send(:file_has_violations?, file_path)).to be true + end + end + end +end diff --git a/tooling/danger/internal_users.rb b/tooling/danger/internal_users.rb new file mode 100644 index 0000000000000000000000000000000000000000..6614ee668fb7f19b1db4bfe5d3ebc6188eb1d51f --- /dev/null +++ b/tooling/danger/internal_users.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require_relative 'suggestor' + +module Tooling + module Danger + module InternalUsers + include ::Tooling::Danger::Suggestor + + DOCS_PATH = 'doc/administration/internal_users.md' + + INLINE_COMMENT = + <<~MESSAGE.freeze + This line modifies internal user behavior or references an internal bot user. + If this introduces new capabilities or changes existing behavior, please update the [internal users documentation](#{DOCS_PATH}). + + This comment will only appear once per file. + MESSAGE + + MR_LEVEL_COMMENT = + <<~SUGGEST_COMMENT.freeze + This MR changes internal user behavior or usage. + Please ensure [internal users documentation](#{DOCS_PATH}) is up to date. + + If this is not applicable, please ignore this message. + + Consider documenting: + - Any changes to internal user behavior or capabilities + - New use cases or integrations + - Changes in how bots may interact with GitLab and its features + SUGGEST_COMMENT + + BOT_METHODS = %w[ + support_bot + alert_bot + visual_review_bot + ghost + migration_bot + security_bot + automation_bot + security_policy_bot + admin_bot + suggested_reviewers_bot + llm_bot + placeholder + duo_code_review_bot + import_user + ].freeze + + BOT_NAMES = BOT_METHODS.join('|') + + BOT_METHOD_DEF_REGEX = /^\s*def\s+(?:self\.)?\s*(#{BOT_NAMES})(?!\w)/ + BOT_MODULE_CALL_REGEX = /^\s*Users::Internal\.(?:#{BOT_NAMES})(?!\w)/ + BOT_SYMBOL_TYPE_REGEX = /^\s*(?!#|")[^#"]*:(?:#{BOT_NAMES})(?!\w)/ + + def add_comment_for_internal_users_changes + return if helper.all_changed_files.include?(DOCS_PATH) + + violations_found = false + + helper.all_changed_files.each do |filename| + next unless filename.end_with?('.rb') + next unless file_has_violations?(filename) + + violations_found = true + markdown(INLINE_COMMENT, file: filename) + end + + warn(MR_LEVEL_COMMENT) if violations_found + end + + private + + def file_has_violations?(filename) + changed_lines = helper.changed_lines(filename) + file_lines = project_helper.file_lines(filename) + + in_bot_method = false + has_violation = false + + changed_lines.each do |line| + clean_line = line.delete_prefix('+').delete_prefix('-').strip + next unless clean_line.match?(BOT_METHOD_DEF_REGEX) || + clean_line.match?(BOT_MODULE_CALL_REGEX) || + clean_line.match?(BOT_SYMBOL_TYPE_REGEX) + + has_violation = true + break + end + + return has_violation if has_violation + + file_lines.each_with_index do |line, _| + if line.match?(BOT_METHOD_DEF_REGEX) + in_bot_method = true + elsif line.strip == 'end' + in_bot_method = false + end + + next unless in_bot_method + next unless changed_lines.any? { |cl| cl.delete_prefix('+').delete_prefix('-').strip == line.strip } + + has_violation = true + break + end + + has_violation + end + end + end +end