Bail out of a flow trigger run for an unsupported resource type

Ai::FlowTriggers::RunService would happily start a flow for a resource type the flow cannot handle. recommend_reviewers/v1 declares no supported_events, so an @duo mention on an issue started a workflow whose goal was the flow's description sentence bound to merge_request_iid - the run then did nothing.

Foundational flows can now declare supported_resource_types, and RunService bails with a validation error before any workflow is created when the trigger resource is not one of them. A nil declaration means "no restriction", so only recommend_reviewers/v1 changes behaviour.

Behind the bail_flow_trigger_on_unsupported_resource feature flag, disabled by default.

Related to #612121 (closed)

Detailed context for AI agents

Why the existing attributes did not work

Ai::Catalog::FoundationalFlow had no notion of an accepted resource type, and the two adjacent attributes are the wrong axis:

  • supported_events is event-shaped. It is nil for recommend_reviewers/v1, and nil means any event is allowed (Ai::FlowTrigger#supported_events_match_foundational_flow). Narrowing it to merge-request-ish events would still be wrong - a mention can land on an issue or a merge request, so the event does not tell you the resource.
  • precondition is a Gitlab::FilterEvaluator filter run against the webhook payload. Expressing "the resource must be a MergeRequest" there means one rule per payload shape the flow can receive, kept in sync by hand.

What changed

File Change
ee/app/models/ai/catalog/foundational_flow/attributes.rb New supported_resource_types attribute. nil means no restriction, [] means no trigger can ever start the flow.
ee/app/models/ai/catalog/foundational_flow.rb #supports_resource?(resource) - true when the attribute is nil, otherwise is_a? against each declared class.
ee/app/services/ai/flow_triggers/run_service.rb Guard as the first check in #validation_error, returning reason: :unsupported_resource_type.
ee/app/models/ai/catalog/foundational_flow/definitions/recommend_reviewers.rb Declares supported_resource_types: [::MergeRequest].
ee/app/services/ai/messaging/adapters/gitlab_duo_note.rb #error_text branch for the new reason.

Where the guard sits and why

RunService#validation_error is evaluated inside execute_flow, before start_catalog_workflow / run_workload, so nothing is created. The check runs ahead of the autonomous_trigger? branch: whether a flow can handle a resource is independent of whether a human or a service account pulled the trigger.

Both mention paths handle an error ServiceResponse without raising, so the mention loop in Notes::PostProcessService stays intact and sibling triggers are not re-run on NewNoteWorker retry:

  • Adapter path (all foundational flows, including this one): route_mention_through_adapter? sends them through Ai::Messaging::Adapters::GitlabDuoNote. with_lifecycle_hooks calls on_flow_failed with response.reason. The issue description said CreateNoteService handles this - that is no longer the path foundational flows take, which is why error_text needed the new branch. Without it the user would have seen "Something went wrong. Please try again."
  • CreateNoteService path (non-foundational catalog flows): mark_failed(note, response.message) flips the progress note to the error text.

Non-mention callers (EE::IssuableBaseService#execute_flow_triggers, Ai::FlowTriggers::EventTriggerService, the CloudEventsFlowTriggerWorker concern, Ai::DailyFlowOnPushWorker) discard the response, so the bail is silent there - correct, since there is no thread to reply into.

Feature flag

bail_flow_trigger_on_unsupported_resource, gitlab_com_derisk, project actor, default off. Rollout issue: #624218

Nothing outside the flag can execute or fail. Two additions sit outside it, and neither is reachable with the flag off:

  • The supported_resource_types attribute, #supports_resource? and the [::MergeRequest] declaration are inert data whose only reader is the gated #supported_resource?. Nothing serialises the attribute set - SeedFoundationalFlowsService assigns an explicit column list, and there is no GraphQL, REST or serializer exposure of FoundationalFlow attributes.
  • The adapter's error_text branch only fires on :unsupported_resource_type, and the gated guard is the only producer of that reason.

An earlier revision validated the declaration shape in the model. That is now a spec instead, because the validation was the one thing the flag could not cover and its failure mode was worse than no check at all: load_items! is lazy rather than boot-time, and it writes into storage as it iterates, so an invalid definition raises on first access and then leaves storage non-empty. Every later access skips loading and silently returns a truncated catalog - measured at 7 of 13 flows on a GDK, with recommend_reviewers/v1 and five unrelated flows simply absent. The spec catches the same mistake in CI and names the offending flow.

nil resource

A flow that declares supported_resource_types also rejects resource: nil, because every such flow reads the resource to build its goal - GoalTemplates::RecommendReviewers.resolve raises ArgumentError on nil today. Three callers pass nil, but none of them target a flow that declares the attribute.

The two pre-existing MergeRequest guards

Added by !248388 (merged) and both kept:

  • GoalTemplates::RecommendReviewers.resolve - only reachable from RunService, so it is dead while the flag is on. Kept as the backstop for the flag-off path; comment updated to say so.
  • additional_context_resolver - resolve_additional_context_for is also called from Ai::DuoWorkflows::CreateAndStartWorkflowService, which RunService does not gate, so this one is still load-bearing.

Verification

Run locally, all green:

  • ee/spec/services/ai/flow_triggers/run_service_spec.rb - 148 examples
  • ee/spec/models/ai/catalog/foundational_flow_spec.rb - 116 examples
  • ee/spec/services/ai/messaging/adapters/gitlab_duo_note_spec.rb, ee/spec/models/ai/catalog/goal_templates/recommend_reviewers_spec.rb - 39 examples
  • ee/spec/models/ai/flow_trigger_spec.rb - 114 examples
  • ee/spec/services/ee/notes/post_process_service_spec.rb - 42 examples

New coverage: unsupported resource does not reach Ai::Catalog::Flows::ExecuteService and creates no workflow; the error message and :unsupported_resource_type reason; nil resource; a supported resource still runs; and the flag-off path still runs the unsupported resource.

Manual verification on a GDK

The flag was toggled for real with Feature.enable / Feature.disable in separate processes (no stub_feature_flags), against a real group, project with a repository, AI catalog item, service account, parent and child item consumers, Ai::FlowTrigger, plus a real Issue and MergeRequest:

resource flag off flag on
Issue supported_resource? true, validation_error nil, reaches ExecuteService false, "cannot be triggered for a Issue resource" / :unsupported_resource_type, 0 workflow rows
MergeRequest true, nil, reaches ExecuteService identical - true, nil
nil true, nil, then raises ArgumentError: resource must not be nil false, "cannot be triggered for a missing resource"

The MergeRequest row is byte-identical in both states, so the guard only filters unsupported resources. The nil row also confirms the flag converts a raise into a clean ServiceResponse. Ai::DuoWorkflow.available? is false on a stock GDK, so both states stop at the same Duo permission check downstream of the guard and no run reaches Duo Workflow Service.

One pre-existing example from !248388 (merged) (run_service_spec.rb, "with a template that declines the resource type") asserted that the goal template declines gracefully rather than raising. The guard now stops that run before the template is reached, so the example was moved under stub_feature_flags(bail_flow_trigger_on_unsupported_resource: false) - which is exactly the backstop it was written to cover.

Out of scope

  • Declaring supported_resource_types on any other flow. fix_pipeline/v1 is the obvious next candidate ([::Ci::Pipeline]), but it already constrains itself through supported_events and a precondition, so it gains little and would change behaviour for a flow that is mid-rollout.
  • Removing the goal-template backstop. That belongs in the flag cleanup MR.

Epic: gitlab-org#23019

Edited by Marc Shaw

Merge request reports

Loading
Loading