From b8bce3b82cf806671286b3cc8741c6b6e9615e14 Mon Sep 17 00:00:00 2001 From: Wanderson Policarpo Date: Fri, 3 Oct 2025 15:58:05 +0100 Subject: [PATCH 1/4] Set default GIT_DEPTH to 1 in Duo Workflow jobs definition --- .../duo_workflows/start_workflow_service.rb | 13 +++++- ee/lib/api/ai/duo_workflows/workflows.rb | 15 ++++-- .../api/ai/duo_workflows/workflows_spec.rb | 21 +++++++++ .../start_workflow_service_spec.rb | 46 +++++++++++++++++++ 4 files changed, 90 insertions(+), 5 deletions(-) diff --git a/ee/app/services/ai/duo_workflows/start_workflow_service.rb b/ee/app/services/ai/duo_workflows/start_workflow_service.rb index 117e2003509841..220e8b65206c7f 100644 --- a/ee/app/services/ai/duo_workflows/start_workflow_service.rb +++ b/ee/app/services/ai/duo_workflows/start_workflow_service.rb @@ -4,6 +4,9 @@ module Ai module DuoWorkflows class StartWorkflowService IMAGE = 'registry.gitlab.com/gitlab-org/duo-workflow/default-docker-image/workflow-generic-image:v0.0.4' + DEFAULT_CI_VARIABLES = { + GIT_DEPTH: 1 + }.freeze def initialize(workflow:, params:) @workflow = workflow @@ -86,8 +89,14 @@ def setup_script_commands duo_config.setup_script || [] end + def workflow_variables + ci_variables = @params[:ci_variables].presence || {} + DEFAULT_CI_VARIABLES.merge(ci_variables.symbolize_keys) + end + def variables - { + # custom variables have lower precedence than the ones below + workflow_variables.merge.merge( DUO_WORKFLOW_ADDITIONAL_CONTEXT_CONTENT: serialized_flow_additional_context, DUO_WORKFLOW_BASE_PATH: './', DUO_WORKFLOW_DEFINITION: @params[:workflow_definition], @@ -110,7 +119,7 @@ def variables DUO_WORKFLOW_METADATA: @params[:workflow_metadata], GITLAB_BASE_URL: Gitlab.config.gitlab.url, AGENT_PLATFORM_GITLAB_VERSION: Gitlab.version_info.to_s - } + ) end def commands diff --git a/ee/lib/api/ai/duo_workflows/workflows.rb b/ee/lib/api/ai/duo_workflows/workflows.rb index 9174dc193bd0bd..257f102a584ded 100644 --- a/ee/lib/api/ai/duo_workflows/workflows.rb +++ b/ee/lib/api/ai/duo_workflows/workflows.rb @@ -78,7 +78,8 @@ def start_workflow_params(workflow_id) use_service_account: token_service.use_service_account?, source_branch: params[:source_branch], additional_context: params[:additional_context], - workflow_metadata: Gitlab::DuoWorkflow::Client.metadata(current_user).to_json + workflow_metadata: Gitlab::DuoWorkflow::Client.metadata(current_user).to_json, + ci_variables: params[:ci_variables] } end @@ -121,8 +122,13 @@ def duo_workflow_list_tools end def create_workflow_params - wrkf_params = declared_params(include_missing: false).except(:start_workflow, :source_branch, - :additional_context) + wrkf_params = declared_params(include_missing: false).except( + :start_workflow, + :source_branch, + :additional_context, + :ci_variables + ) + return wrkf_params unless wrkf_params[:ai_catalog_item_version_id] wrkf_params[:ai_catalog_item_version] = ::Ai::Catalog::ItemVersion @@ -179,6 +185,9 @@ def create_workflow_params example: '[{"Category": "agent_user_environment", "Content": "{\"merge_request_url\": ' \ 'https://gitlab.com/project/-/merge_requests/1\"}", "Metadata": "{}"}]' } + optional :ci_variables, type: Hash, + desc: 'The environment variables to pass to the workflow.', + documentation: { example: '{"GIT_DEPTH": 30}' } exactly_one_of :project_id, :namespace_id end end diff --git a/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb b/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb index fc4975b28a10c8..73ad11e65f3ab9 100644 --- a/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb +++ b/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb @@ -573,6 +573,27 @@ expect(response).to have_gitlab_http_status(:bad_request) end end + + context 'when ci_variables are provided' do + let(:ci_variables) do + { 'GIT_DEPTH' => '15' } + end + + let(:params) do + super().merge(ci_variables: ci_variables) + end + + it 'passes ci_variables to StartWorkflowService' do + expect(::Ai::DuoWorkflows::StartWorkflowService).to receive(:new).with( + workflow: anything, + params: hash_including(ci_variables: ci_variables) + ).and_call_original + + post api(path, user), params: params + + expect(response).to have_gitlab_http_status(:created) + end + end end end diff --git a/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb b/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb index 0290ee3a7fdf1b..22c3e2e401ff20 100644 --- a/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb +++ b/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb @@ -170,6 +170,13 @@ end end + shared_context 'with Duo enabled' do + before do + allow(::Gitlab::Llm::StageCheck).to receive(:available?).with(project, :duo_workflow).and_return(true) + allow(maintainer).to receive(:allowed_to_use?).and_return(true) + end + end + subject(:execute) { described_class.new(workflow: workflow, params: params).execute } context 'with workflow enablement checks' do @@ -787,4 +794,43 @@ end end end + + context 'without custom CI variables', :aggregate_failures do + include_context 'with Duo enabled' + + it 'uses default variables' do + expect(Ci::Workloads::RunWorkloadService).to receive(:new).and_wrap_original do |method, **kwargs| + workload_definition = kwargs[:workload_definition] + variables = workload_definition.variables + + expect(variables[:GIT_DEPTH]).to eq(1) + + method.call(**kwargs) + end + + expect(execute).to be_success + end + end + + context 'with custom CI variables' do + include_context 'with Duo enabled' + + let(:params) do + super().merge(ci_variables: { 'GIT_DEPTH' => 15, 'GIT_STRATEGY' => 'fetch' }) + end + + it 'merges with default variables' do + expect(Ci::Workloads::RunWorkloadService).to receive(:new).and_wrap_original do |method, **kwargs| + workload_definition = kwargs[:workload_definition] + variables = workload_definition.variables + + expect(variables[:GIT_DEPTH]).to eq(15) + expect(variables[:GIT_STRATEGY]).to eq('fetch') + + method.call(**kwargs) + end + + expect(execute).to be_success + end + end end -- GitLab From 96baf2dbe6fe44ee507bc4869aaf072d36d96ab0 Mon Sep 17 00:00:00 2001 From: Wanderson Policarpo Date: Fri, 3 Oct 2025 16:57:45 +0000 Subject: [PATCH 2/4] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Kinshuk Singh --- ee/app/services/ai/duo_workflows/start_workflow_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/ai/duo_workflows/start_workflow_service.rb b/ee/app/services/ai/duo_workflows/start_workflow_service.rb index 220e8b65206c7f..786100084cf332 100644 --- a/ee/app/services/ai/duo_workflows/start_workflow_service.rb +++ b/ee/app/services/ai/duo_workflows/start_workflow_service.rb @@ -96,7 +96,7 @@ def workflow_variables def variables # custom variables have lower precedence than the ones below - workflow_variables.merge.merge( + workflow_variables.merge( DUO_WORKFLOW_ADDITIONAL_CONTEXT_CONTENT: serialized_flow_additional_context, DUO_WORKFLOW_BASE_PATH: './', DUO_WORKFLOW_DEFINITION: @params[:workflow_definition], -- GitLab From cc044dbd05065b913fb34a05940ddae5bbb74d3d Mon Sep 17 00:00:00 2001 From: Wanderson Policarpo Date: Fri, 3 Oct 2025 18:46:30 +0100 Subject: [PATCH 3/4] Improve comment on workflow variables to avoid confusion --- .../ai/duo_workflows/start_workflow_service.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/ee/app/services/ai/duo_workflows/start_workflow_service.rb b/ee/app/services/ai/duo_workflows/start_workflow_service.rb index 786100084cf332..f3bd369a4082de 100644 --- a/ee/app/services/ai/duo_workflows/start_workflow_service.rb +++ b/ee/app/services/ai/duo_workflows/start_workflow_service.rb @@ -94,9 +94,8 @@ def workflow_variables DEFAULT_CI_VARIABLES.merge(ci_variables.symbolize_keys) end - def variables - # custom variables have lower precedence than the ones below - workflow_variables.merge( + def fixed_variables + { DUO_WORKFLOW_ADDITIONAL_CONTEXT_CONTENT: serialized_flow_additional_context, DUO_WORKFLOW_BASE_PATH: './', DUO_WORKFLOW_DEFINITION: @params[:workflow_definition], @@ -119,7 +118,13 @@ def variables DUO_WORKFLOW_METADATA: @params[:workflow_metadata], GITLAB_BASE_URL: Gitlab.config.gitlab.url, AGENT_PLATFORM_GITLAB_VERSION: Gitlab.version_info.to_s - ) + } + end + + def variables + # Fixed variables have higher precedence than workflow variables. + # This is to avoid giving too much control to end users when creating a workflow job. + workflow_variables.merge(fixed_variables) end def commands -- GitLab From 957c528e52eb4dcae1569a13d418cc753ab3056d Mon Sep 17 00:00:00 2001 From: Wanderson Policarpo Date: Mon, 6 Oct 2025 14:37:48 +0100 Subject: [PATCH 4/4] Sanitize workflow job variables before creating a job --- .../sanitize_job_variables_service.rb | 38 ++++++++++++ .../duo_workflows/start_workflow_service.rb | 60 +++++++++++-------- ee/lib/api/ai/duo_workflows/workflows.rb | 17 ++++-- .../api/ai/duo_workflows/workflows_spec.rb | 29 +++++++-- .../sanitize_job_variables_service_spec.rb | 55 +++++++++++++++++ .../start_workflow_service_spec.rb | 24 ++++++-- 6 files changed, 186 insertions(+), 37 deletions(-) create mode 100644 ee/app/services/ai/duo_workflows/sanitize_job_variables_service.rb create mode 100644 ee/spec/services/ai/duo_workflows/sanitize_job_variables_service_spec.rb diff --git a/ee/app/services/ai/duo_workflows/sanitize_job_variables_service.rb b/ee/app/services/ai/duo_workflows/sanitize_job_variables_service.rb new file mode 100644 index 00000000000000..a12ec8e76902ab --- /dev/null +++ b/ee/app/services/ai/duo_workflows/sanitize_job_variables_service.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Ai + module DuoWorkflows + class SanitizeJobVariablesService + OVERRIDABLE_JOB_VARIABLES = { + GIT_DEPTH: 1 + }.freeze + + def initialize(dirty_variables) + @dirty_variables = (dirty_variables.presence || {}).symbolize_keys + end + + def execute + invalid_variables = dirty_variables.keys - OVERRIDABLE_JOB_VARIABLES.keys + + if invalid_variables.any? + return ServiceResponse.error( + message: <<~MESSAGE.squish, + Non-overridable variables detected: #{invalid_variables.to_sentence}. + Allowed variables: #{OVERRIDABLE_JOB_VARIABLES.keys.to_sentence}. + MESSAGE + reason: :unprocessable_entity) + end + + sanitized_variables = OVERRIDABLE_JOB_VARIABLES.each_with_object({}) do |(key, value), variables| + variables[key] = dirty_variables.fetch(key, value) + end + + ServiceResponse.success(payload: { variables: sanitized_variables }) + end + + private + + attr_reader :dirty_variables + end + end +end diff --git a/ee/app/services/ai/duo_workflows/start_workflow_service.rb b/ee/app/services/ai/duo_workflows/start_workflow_service.rb index f3bd369a4082de..cf017bcd37b22c 100644 --- a/ee/app/services/ai/duo_workflows/start_workflow_service.rb +++ b/ee/app/services/ai/duo_workflows/start_workflow_service.rb @@ -3,28 +3,19 @@ module Ai module DuoWorkflows class StartWorkflowService + include ::Gitlab::Utils::StrongMemoize + IMAGE = 'registry.gitlab.com/gitlab-org/duo-workflow/default-docker-image/workflow-generic-image:v0.0.4' - DEFAULT_CI_VARIABLES = { - GIT_DEPTH: 1 - }.freeze def initialize(workflow:, params:) @workflow = workflow @current_user = workflow.user @params = params + @job_variables = (params.delete(:job_variables) || {}).symbolize_keys end def execute - unless @workflow.project_level? - return ServiceResponse.error( - message: 'Only project-level workflow is supported', - reason: :unprocessable_entity) - end - - unless @current_user.can?(:execute_duo_workflow_in_ci, @workflow) - return ServiceResponse.error(message: 'Can not execute workflow in CI', - reason: :feature_unavailable) - end + return validation if validation.error? workload_user = @current_user @@ -58,6 +49,33 @@ def execute private + def validation + sanitized_variables = ::Ai::DuoWorkflows::SanitizeJobVariablesService.new(@job_variables).execute + + if sanitized_variables.error? + return ServiceResponse.error( + message: sanitized_variables.message, + reason: :unprocessable_entity) + end + + unless @workflow.project_level? + return ServiceResponse.error( + message: 'Only project-level workflow is supported', + reason: :unprocessable_entity + ) + end + + unless @current_user.can?(:execute_duo_workflow_in_ci, @workflow) + return ServiceResponse.error( + message: 'Can not execute workflow in CI', + reason: :feature_unavailable + ) + end + + ServiceResponse.success + end + strong_memoize_attr :validation + def workload_definition ::Ci::Workloads::WorkloadDefinition.new do |d| d.image = @workflow.image.presence || configured_image || IMAGE @@ -89,12 +107,12 @@ def setup_script_commands duo_config.setup_script || [] end - def workflow_variables - ci_variables = @params[:ci_variables].presence || {} - DEFAULT_CI_VARIABLES.merge(ci_variables.symbolize_keys) + def sanitized_variables + ::Ai::DuoWorkflows::SanitizeJobVariablesService.new(@job_variables).execute end + strong_memoize_attr :validation - def fixed_variables + def variables { DUO_WORKFLOW_ADDITIONAL_CONTEXT_CONTENT: serialized_flow_additional_context, DUO_WORKFLOW_BASE_PATH: './', @@ -118,13 +136,7 @@ def fixed_variables DUO_WORKFLOW_METADATA: @params[:workflow_metadata], GITLAB_BASE_URL: Gitlab.config.gitlab.url, AGENT_PLATFORM_GITLAB_VERSION: Gitlab.version_info.to_s - } - end - - def variables - # Fixed variables have higher precedence than workflow variables. - # This is to avoid giving too much control to end users when creating a workflow job. - workflow_variables.merge(fixed_variables) + }.merge(sanitized_variables.payload.fetch(:variables)) end def commands diff --git a/ee/lib/api/ai/duo_workflows/workflows.rb b/ee/lib/api/ai/duo_workflows/workflows.rb index 257f102a584ded..e5f22aa37e929d 100644 --- a/ee/lib/api/ai/duo_workflows/workflows.rb +++ b/ee/lib/api/ai/duo_workflows/workflows.rb @@ -17,6 +17,8 @@ class Workflows < ::API::Base end helpers do + include Gitlab::Utils::StrongMemoize + def find_root_namespace # The IDE sends namespace data via the header, while the web agentic chat UI # sends it as a query param. @@ -79,7 +81,7 @@ def start_workflow_params(workflow_id) source_branch: params[:source_branch], additional_context: params[:additional_context], workflow_metadata: Gitlab::DuoWorkflow::Client.metadata(current_user).to_json, - ci_variables: params[:ci_variables] + job_variables: sanitized_variables.payload.fetch(:variables) } end @@ -126,7 +128,7 @@ def create_workflow_params :start_workflow, :source_branch, :additional_context, - :ci_variables + :job_variables ) return wrkf_params unless wrkf_params[:ai_catalog_item_version_id] @@ -136,6 +138,11 @@ def create_workflow_params wrkf_params end + def sanitized_variables + ::Ai::DuoWorkflows::SanitizeJobVariablesService.new(params[:job_variables]).execute + end + strong_memoize_attr :sanitized_variables + params :workflow_params do optional :project_id, type: String, desc: 'The ID or path of the workflow project', documentation: { example: '1' } @@ -185,8 +192,8 @@ def create_workflow_params example: '[{"Category": "agent_user_environment", "Content": "{\"merge_request_url\": ' \ 'https://gitlab.com/project/-/merge_requests/1\"}", "Metadata": "{}"}]' } - optional :ci_variables, type: Hash, - desc: 'The environment variables to pass to the workflow.', + optional :job_variables, type: Hash, + desc: 'Overridable environment variables to pass to the workflow job.', documentation: { example: '{"GIT_DEPTH": 30}' } exactly_one_of :project_id, :namespace_id end @@ -326,6 +333,8 @@ def create_workflow_params 'https://gitlab.com/gitlab-org/gitlab/-/issues/566195', new_threshold: 105 ) + bad_request!(sanitized_variables.message) if sanitized_variables.error? + container = if params[:project_id] find_project!(params[:project_id]) elsif params[:namespace_id] diff --git a/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb b/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb index 73ad11e65f3ab9..0bad54378180e4 100644 --- a/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb +++ b/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb @@ -574,19 +574,19 @@ end end - context 'when ci_variables are provided' do - let(:ci_variables) do + context 'when valid job_variables are provided' do + let(:job_variables) do { 'GIT_DEPTH' => '15' } end let(:params) do - super().merge(ci_variables: ci_variables) + super().merge(job_variables: job_variables) end - it 'passes ci_variables to StartWorkflowService' do + it 'passes job_variables to StartWorkflowService' do expect(::Ai::DuoWorkflows::StartWorkflowService).to receive(:new).with( workflow: anything, - params: hash_including(ci_variables: ci_variables) + params: hash_including(job_variables: { GIT_DEPTH: '15' }) ).and_call_original post api(path, user), params: params @@ -594,6 +594,25 @@ expect(response).to have_gitlab_http_status(:created) end end + + context 'when invalid job_variables are provided' do + let(:job_variables) do + { 'GIT_STRATEGY' => 'fetch' } + end + + let(:params) do + super().merge(job_variables: job_variables) + end + + it 'returns an API error' do + post api(path, user), params: params + + expect(response).to have_gitlab_http_status(:bad_request) + expect(response.body).to include( + 'Non-overridable variables detected: GIT_STRATEGY. Allowed variables: GIT_DEPTH' + ) + end + end end end diff --git a/ee/spec/services/ai/duo_workflows/sanitize_job_variables_service_spec.rb b/ee/spec/services/ai/duo_workflows/sanitize_job_variables_service_spec.rb new file mode 100644 index 00000000000000..047a655a33dafa --- /dev/null +++ b/ee/spec/services/ai/duo_workflows/sanitize_job_variables_service_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ai::DuoWorkflows::SanitizeJobVariablesService, feature_category: :agent_foundations do + subject(:result) { described_class.new(dirty_variables).execute } + + let(:variables) { result.payload.fetch(:variables) } + let(:default_values) { described_class::OVERRIDABLE_JOB_VARIABLES } + + context 'when dirty variables are invalid' do + let(:dirty_variables) { { GIT_STRATEGY: 'fetch' } } + + it 'returns an error' do + expect(result).to be_error + expect(result.message).to eq('Non-overridable variables detected: GIT_STRATEGY. Allowed variables: GIT_DEPTH.') + end + end + + context 'when dirty variables are valid' do + let(:dirty_variables) { { GIT_DEPTH: 15 } } + + it 'returns the sanitized variables' do + expect(result).to be_success + expect(variables).to eq(GIT_DEPTH: 15) + end + end + + context 'when dirty variables keys are string' do + let(:dirty_variables) { { 'GIT_DEPTH' => 15 } } + + it 'sanitize keys to symbols' do + expect(result).to be_success + expect(variables).to eq(GIT_DEPTH: 15) + end + end + + context 'when dirty variables are nil' do + let(:dirty_variables) { nil } + + it 'returns the default values' do + expect(result).to be_success + expect(variables).to eq(default_values) + end + end + + context 'when dirty variables are empty' do + let(:dirty_variables) { {} } + + it 'returns the default values' do + expect(result).to be_success + expect(variables).to eq(default_values) + end + end +end diff --git a/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb b/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb index 22c3e2e401ff20..d9621378a47df0 100644 --- a/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb +++ b/ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb @@ -795,7 +795,7 @@ end end - context 'without custom CI variables', :aggregate_failures do + context 'without custom job variables', :aggregate_failures do include_context 'with Duo enabled' it 'uses default variables' do @@ -812,11 +812,11 @@ end end - context 'with custom CI variables' do + context 'with valid custom job variables' do include_context 'with Duo enabled' let(:params) do - super().merge(ci_variables: { 'GIT_DEPTH' => 15, 'GIT_STRATEGY' => 'fetch' }) + super().merge(job_variables: { 'GIT_DEPTH' => 15 }) end it 'merges with default variables' do @@ -825,7 +825,6 @@ variables = workload_definition.variables expect(variables[:GIT_DEPTH]).to eq(15) - expect(variables[:GIT_STRATEGY]).to eq('fetch') method.call(**kwargs) end @@ -833,4 +832,21 @@ expect(execute).to be_success end end + + context 'with invalid custom job variables' do + include_context 'with Duo enabled' + + let(:params) do + super().merge(job_variables: { 'GIT_DEPTH' => 15, 'GIT_STRATEGY' => 'fetch', 'FOO' => 'bar' }) + end + + it 'does not create a workflow' do + expect(execute).to be_error + expect(execute.reason).to eq(:unprocessable_entity) + expect(execute.message).to eq(<<~MESSAGE.squish) + Non-overridable variables detected: GIT_STRATEGY and FOO. + Allowed variables: GIT_DEPTH. + MESSAGE + end + end end -- GitLab