Delete the bespoke DAP reviewer assignment code path

Deletes the bespoke DAP reviewer-assignment path now that the flow trigger covers it: the DAP branch in AutoAssignReviewersWorker, Ai::DuoWorkflows::RecommendReviewers::ExecuteService, Project#dap_powered_recommend_reviewers_available?, and the dap_powered_recommend_reviewers flag definition.

This now merges before the Phase C row-reset migration (!248480 (closed)), reversing the original epic order, and only after the flag has been turned off so it deletes already-dead code rather than changing behaviour. Both gates below are now met - the earlier audit and notify blockers were dropped. Sequence below.

  • Supersedes !248482 (closed), closed in favour of this one - same branch and same commit
  • Related to #607679 (closed) · Epic: gitlab-org#23019
  • !242698 (merged) merged on 2026-08-28, and merge_request_create_flow_trigger enabled globally on gprd on 2026-09-01 - rollout issue #618743 (closed)
  • dap_powered_recommend_reviewers turned off by chatops on 2026-09-02 (removed on gprd and gstg) - rollout issue #602418 (closed)
  • Production count of project_settings.reviewer_assignment_strategy = 2 - 6 rows, measured by the db:gitlabcom-database-testing run on !248480 (closed) at commit c7f0967e
Detailed context for AI agents

This closes the last acceptance criterion on #603494 (closed) - "The bespoke trigger code introduced in the original epic is removed or replaced".

Removed

Thing Why it's safe
DAP branch in AutoAssignReviewersWorker AssignService becomes the only path
include Gitlab::InternalEventsTracking in that worker Its only use was the deleted branch
Ai::DuoWorkflows::RecommendReviewers::ExecuteService The deleted branch was its only production caller
Project#dap_powered_recommend_reviewers_available? Only callers were the branch and the settings view, removed in !248476 (merged)

Merge ordering

This MR merges before !248480 (closed), reversing the order the epic originally assumed. The only thing that ever forced the migration first was the enum-removal NoMethodError trap: an earlier revision of this MR deleted the dap_powered: 2 enum value, which would have deleted the generated reviewer_assignment_dap_powered? predicate out from under its unconditional caller in the worker. This MR no longer touches the enum, so the trap is gone and the ordering falls back to the general rule - land the revertible code change first, save the irreversible data change for last.

Agreed sequence:

  1. !242698 (merged) merges and its flag goes on. It adds the merge_request created trigger, which restores flow-trigger coverage for merge requests opened non-draft - coverage the bespoke path had and the marked-ready trigger does not. Merging is not sufficient: it ships behind merge_request_create_flow_trigger, gitlab_com_derisk, default off, project-scoped (rollout #618743 (closed)). The flag has to be on for the projects that currently rely on non-draft coverage before the old path goes.
  2. dap_powered_recommend_reviewers is turned off by chatops. Rollout issue #602418 (closed). This is the step where the feature actually stops running in production, and it reverts in seconds if something looks wrong.
  3. This MR merges, deleting what is by then already-dead code.
  4. !248480 (closed) merges and resets the stale rows.

There is deliberately no audit step. Earlier revisions blocked on splitting the affected rows into those that ever functioned versus those merely set, and on notifying the functioning set first. Both were dropped: 6 internal projects on a wip flag that never reached GA does not justify it. Affected teams get a note in the internal feedback thread (#603172), and the replacement setup is documented in !248483 (merged).

Self-managed instances with the flag explicitly enabled

The safety argument above covers gprd and gstg only: the chatops flip on 2026-09-02 removed dap_powered_recommend_reviewers from GitLab.com, not from self-managed instances. If a self-managed instance had explicitly enabled the flag and has a project with reviewer_assignment_strategy set to dap_powered, this MR is a behaviour change at the 19.4 upgrade rather than a dead-code deletion: the feature stops, and those projects fall through to AssignService, which skips with "No strategy available" and assigns nobody. Nothing raises and no data is lost.

The affected population should be effectively nil:

  • the flag was type: wip and default_enabled: false, so an admin had to turn it on deliberately
  • the feature never reached GA
  • it additionally required Duo foundational flows enabled, the recommend_reviewers/v1 catalog item enabled, and a passing Duo Workflow stage check

The replacement setup is documented in doc/user/project/merge_requests/reviews/automatic_reviewer_assignment.md.

Kept, and why removing it was not backwards compatible

dap_powered: 2 enum value - kept

Removing it does not corrupt anything, but it silently breaks a documented API contract.

Measured against a row holding raw 2 with the enum value deleted:

Check Result
raw column after the change 2 (preserved)
setting.reviewer_assignment_strategy nil
setting.valid? true, no errors
update! of an unrelated attribute succeeds, raw value still 2, no NOT NULL violation
reviewer_auto_assignment_enabled? true (nil != 'disabled')

So no crash and no data loss. The problem is the API surface: EE::API::Entities::Project exposes reviewer_assignment_strategy straight off the model, so GET /projects/:id would start returning null for those projects. !248476 (merged) added a sentence to doc/api/projects.md in four places saying the attribute "can also return dap_powered for projects configured before GitLab 19.4". Dropping the enum makes that statement false without touching the docs.

Keeping the enum costs nothing behaviourally. With the DAP branch gone, an existing dap_powered row degrades identically either way - verified end to end:

strategy reads as: "dap_powered"
reviewer_auto_assignment_enabled?: true
StrategyFactory.build: nil
AssignService: status=:success message="No strategy available"
worker perform: no raise; reviewers=[]

StrategyFactory::STRATEGIES only maps code_owners, so both "dap_powered" and nil fall through to the same skipped success. The one wrinkle worth knowing either way: because reviewer_auto_assignment_enabled? compares against the string 'disabled', these projects keep enqueuing AutoAssignReviewersWorker on every create and ready transition, and it no-ops. Wasteful, not broken.

Removing the enum is a separate later cleanup. It has to carry a doc/api/projects.md correction in those four places, and an update to ee/spec/views/projects/settings/merge_requests/_reviewer_auto_assignment_settings.html.haml_spec.rb:62, which calls build(:project_setting, reviewer_assignment_strategy: :dap_powered) and will raise ArgumentError the moment the value goes.

dap_powered_recommend_reviewers flag definition - now deleted

An earlier version of this description said the definition was kept because #607676 (closed) needed it. That is stale: #607676 (closed) has since closed as "no gate needed", so the reasoning no longer applies and the diff deletes ee/config/feature_flags/wip/dap_powered_recommend_reviewers.yml.

Verification on #607676 (closed) showed the flag never gated catalog visibility or execution in the first place. Ai::Catalog::FoundationalFlow#blocked_by_feature_flag? is a no-op unless the flow definition declares a feature_flag: attribute, and recommend_reviewers/v1 does not - unlike business_context_security_guidelines (sdlc_context_agent_trigger) and risk_classification (duo_mr_risk_classification). So the Recommend Reviewers flow was already ungated on master regardless of this flag's state, and the trigger path works without it.

The flag was only ever read through Project#dap_powered_recommend_reviewers_available?, which this MR deletes. With that gone nothing reads it, and the rspec:feature-flags job fails on a flag definition with no readers - so keeping the definition was not an option either way.

One note for anyone revisiting a catalog-level gate later: blocked_by_feature_flag? evaluates with a group actor, while dap_powered_recommend_reviewers was rolled out with a project actor. A future gate should be a fresh flag rather than a reuse of this name.

Kept - verified still needed

  • ReviewerDataBuilder - now reached only from the additional_context_resolver on the recommend_reviewers/v1 catalog item (ee/app/models/ai/catalog/foundational_flow/definitions/recommend_reviewers.rb), which is the trigger path.
  • The recommend_reviewers/v1 catalog item - it's the product now.
  • The auto_assign_reviewers internal event - still fired by AssignService with label: 'code_owners', and the label description stays accurate.
  • The project_settings.reviewer_assignment_strategy column.

Flag reach, for whoever does the chatops flip

The rollout issue records the flag being set on the gitlab-org group scope on 2026-06-16, but the code only ever checked it with a project actor, and there is no group-to-project cascade in FeatureGate#flipper_id or Feature.enabled?. So that group-scoped flip may never have affected gating at all. The enablements that plausibly did bite are the project-scoped ones: gitlab-com/create-stage/code-review-ai-experiment-playground (2026-06-09) and gitlab-org/customers-gitlab-com (2026-06-16). Confirm current state with /chatops run feature get dap_powered_recommend_reviewers before flipping it off - the functioning set may be 1 or 2 projects rather than all 6 rows.

Stale rows

Production holds 6 rows with project_settings.reviewer_assignment_strategy = 2, measured by the db:gitlabcom-database-testing run on !248480 (closed) at commit c7f0967e against the gitlab.com production main clone. They are internal projects.

Note the ordering consequence: those projects stop getting DAP assignment at step 2, the flag flip, not when this MR merges. By the time this lands the path is already inert, which is the point of putting the flip first.

How many of the 6 ever functioned is not being measured. Some fraction were set through the never-flag-gated PUT /projects/:id write path that !248476 (merged) closed, and never executed anything at all. The gate that would have told them apart, Project#dap_powered_recommend_reviewers_available?, is deleted by this MR and reads a flag that is off by step 2, so it is not reconstructable afterwards. Accepted.

Measurement

Nothing to hand off, and nothing breaks. An earlier revision carried this as an open acceptance criterion ("record the measurement hand-off in the rollout notes"); it has been dropped as bookkeeping for a metric that was never built.

Verified on master: no metric definition consumes auto_assign_reviewers - nothing in config/metrics/ or ee/config/metrics/ references it. The only references anywhere are the event definition (ee/config/events/auto_assign_reviewers.yml) and the two call sites. So deleting the worker's label: 'dap_powered' call breaks no dashboard, because there was no dashboard.

The event itself also survives: AssignService still fires it with label: 'code_owners', and the label description ("The assignment strategy used") stays accurate.

If anyone does want to measure trigger-driven reviewer assignment later, the signal is trigger_ai_catalog_item, emitted per run with the catalog item id, filtered to the Recommend Reviewers item.

Docs

Already handled. doc/user/project/merge_requests/reviews/automatic_reviewer_assignment.md was rewritten by !248483 (merged), merged 2026-08-26, and its history section on master already reads "Feature flag dap_powered_recommend_reviewers removed" in 19.4. The docs are ahead of the code; this MR is what makes that sentence true.

Notes for review

  • MergeRequests::ExecuteMergeRequestReadyWorker is deliberately not deleted here. It is an unrelated no-op left over from the 19.1 CloudEvent refactor, and #602537 scopes its removal as two steps: a sidekiq_remove_jobs migration first, then the class. Deleting the class here would do that issue's second half without its first, so both go together in #602537 and this MR stays purely the DAP path.
  • The optional adjacent cleanup was not taken. app/models/project_setting.rb carries ignore_column :code_owner_reviewer_assignment_strategy, remove_with: '19.0', remove_after: '2026-04-22', past its remove-after date. Unrelated to DAP, so per the minimal-fix rule it gets its own MR.

Testing

Specs Examples
auto_assign_reviewers_worker_spec.rb + _reviewer_auto_assignment_settings.html.haml_spec.rb + spec/models/project_setting_spec.rb 105, 0 failures
ee/spec/services/merge_requests/reviewer_assignment/ + ee/spec/services/ai/duo_workflows/recommend_reviewers/ 61, 0 failures

ee/spec/models/ee/project_spec.rb loads clean after the #dap_powered_recommend_reviewers_available? block was removed. RuboCop, gitlab:sidekiq:queues:check, unused-code-linter and openapi checks pass in the pre-push hooks.

The two behaviour tables above were produced by throwaway probe specs that inserted a raw 2 and exercised read, validate, save, the strategy factory, AssignService, and AutoAssignReviewersWorker#perform. They are not committed.

New regression spec for the inert-row path

The dap_powered context in auto_assign_reviewers_worker_spec.rb tested the DAP-execution branch this MR deletes, so the whole context goes. That left the inert-row case unpinned - a row that still says dap_powered after the bespoke path is gone - so a replacement context was added: "when reviewer_assignment_strategy is still dap_powered". It asserts two things:

  1. The worker reaches AssignService and calls it for real (and_call_original), raises nothing, and assigns nobody.
  2. AssignService returns a success whose message is exactly 'No strategy available' - pinning that it skips for want of a strategy rather than bailing earlier on some unrelated skip_reason such as 'Feature disabled'.

The safety here is structural rather than special-cased: StrategyFactory::STRATEGIES maps only 'code_owners', so a hash lookup on 'dap_powered' misses and build returns nil. The same miss happens if the enum is later dropped and the raw value deserializes to nil instead, so both routes reach the same skip. The context's before block also doubles as a tripwire for that later enum removal, since it writes the string 'dap_powered' and would raise ArgumentError the moment the enum value goes.

MR acceptance checklist

  • Follows the code review guidelines
  • Tests updated; no coverage lost for the code_owners path
  • No migration in this MR
Edited by Marc Shaw

Merge request reports

Loading
Loading