Draft: Scope foundational flow preconditions per event type and skip reviewer recommendation when reviewers exist

Marc Shawrequested to merge
607907-trigger-path-guardrails into master

What does this MR do and why?

MergeRequests::AutoAssignReviewersWorker returns early when a merge request already has a non-automated reviewer, so auto-assignment never overwrites reviewers a human deliberately chose (auto_assign_reviewers_worker.rb:19, 61-69). The DAP trigger path has no equivalent, so marking ready a merge request that already has reviewers launches a Recommend Reviewers run that can churn them. This MR restores that guardrail on the trigger path.

Why a flow precondition isn't enough on its own

The obvious fix is a foundational flow precondition on reviewer counts. It doesn't work, for two reasons:

  1. Preconditions apply to every event type a flow supports, and Gitlab::FilterEvaluator is fail-closed — dig_value raises on a missing intermediate key and evaluate rescues to false. A rule reading reviewers.human_count would therefore silently block every run of any other event type this flow is configured for, because those events don't supply the field.
  2. The guard is semantically wrong for some events. recommend_reviewers/v1 has no supported_events restriction, so it can be wired to mention. If someone comments "@duo recommend reviewers" on a merge request that already has reviewers, they've explicitly asked — refusing is the wrong behaviour.

Narrowing supported_events to ready-only would dodge both, but it removes configurability and would make the mention branch of the goal template added in !248388 (merged) dead code. So instead this MR fixes the underlying platform limitation.

Event-scoped preconditions

event_preconditions is a new foundational flow attribute with the same shape as precondition, but keyed by event type name and applied only when that event fires. An event with no entry is left unfiltered rather than blocked.

  • The existing flow-wide precondition is unchanged and still applies to every event, so fix_pipeline/v1 behaves exactly as before.
  • Both apply when both match, ANDed together with the trigger's own filter. Ai::FlowTriggers::FilterEvaluator#merge_filters was generalised from two filters to N; the nesting depth it produces is unchanged, so Gitlab::FilterEvaluator::MAX_DEPTH is unaffected.

recommend_reviewers/v1 then declares:

event_preconditions: {
  'merge_request_ready' => {
    'match' => 'all',
    'rules' => [{ 'field' => 'reviewers.human_count', 'operator' => 'eq', 'value' => 0 }]
  }
}

and the ready worker supplies that field through a new overridable event_filter_data(resource) hook on Ai::CloudEventsFlowTriggerWorker (defaults to {}, so no other worker changes behaviour). A lone Duo Code Review bot counts as "no reviewer yet", matching non_automated_reviewers_present? exactly for every reachable reviewer combination.

Scope: guard 2 is deliberately not included

#607907 (closed) also covers re-fire suppression (every draft ⇄ ready toggle starts a full run). That is not in this MR, deliberately: nothing in the trigger platform dedups concurrent runs for any flow, so it's a platform gap rather than a Recommend Reviewers one, and fixing it here would mean this flow paying for a platform-wide problem. The issue's own AC allows recording the cost as accepted instead. Recommend following up with a dedicated platform issue modelled on TriggerResolveDependencyBumpWorkflowWorker#skip_due_to_workflow_state?, which already does exactly this for one flow.

References

Notes for review

  • Touches the DAP trigger platform, not just our flow: FilterEvaluator, the FoundationalFlow attribute surface, and the shared worker concern. Worth a group::duo agent platform look alongside code review.
  • Minor conflict with !248388 (merged) — both add keys to the same recommend_reviewers/v1 hash in items.rb. Adjacent but distinct lines; trivial to resolve for whichever lands second.
  • event_preconditions is not exposed over GraphQL. Types::Ai::FlowTriggerType exposes foundational_flow_precondition only, so the UI won't surface this new condition. Left out as it isn't needed for the guard to work — happy to add it if the frontend displays preconditions to users.
  • Pre-existing quirk found while writing specs (not fixed here): Gitlab::FilterEvaluator#dig_value uses value[key] || value[key.to_sym], so a legitimately false field value falls through to the symbol lookup and reads as nil. That makes eq false rules unmatchable. It's in shared code used by webhooks and integrations, so it's out of scope for this MR, but it's worth its own issue.

Testing

bundle exec rspec ee/spec/services/ai/flow_triggers/filter_evaluator_spec.rb \
  ee/spec/workers/ai/catalog/flows/ \
  ee/spec/models/ai/catalog/foundational_flow_spec.rb \
  ee/spec/models/ai/flow_trigger_spec.rb

All green; RuboCop clean on all changed files. The new examples were confirmed to fail without the production changes (11 failures on master). Coverage includes:

  • Event precondition met / not met / declared for a different event than the one firing (the property that removes the fail-closed trap).

  • Flow-wide and event precondition together, each failing independently.

  • The full guardrail parity matrix ported from auto_assign_reviewers_worker_spec.rb: no reviewers, lone Duo bot, one human, human + bot.

  • End-to-end through handle_event with the real FilterEvaluator and a real recommend_reviewers/v1 trigger, asserting RunService is and isn't invoked.

  • All other workers including the shared concern, to confirm the event_filter_data default is a no-op.

  • Verify on a real merge request in the playground project before this leaves Draft.

MR acceptance checklist

  • Tests added, including the unchanged-behaviour paths for other flows and events
  • Follows the code review guidelines
  • No documentation change needed (no user-facing config or API surface; event_preconditions is an internal flow attribute)
  • No migration or database review needed
Edited by Marc Shaw

Merge request reports