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?, the deprecated no-op MergeRequests::ExecuteMergeRequestReadyWorker, 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.
- Related to #607679 (closed) · Epic: gitlab-org#23019
- !242698 (merged) merged on 2026-08-28, and
merge_request_create_flow_triggerenabled globally ongprdon 2026-09-01 - rollout issue #618743 (closed) -
dap_powered_recommend_reviewersturned off by chatops on 2026-09-02 (removed ongprdandgstg) - rollout issue #602418 (closed) - Production count of
project_settings.reviewer_assignment_strategy = 2- 6 rows, measured by thedb:gitlabcom-database-testingrun on !248480 (closed) at commitc7f0967e
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) |
MergeRequests::ExecuteMergeRequestReadyWorker |
Deprecated no-op, not subscribed anywhere - folds in #602537 |
Queue entries were regenerated with gitlab:sidekiq:all_queues_yml:generate and sidekiq_queues_yml:generate rather than hand-edited - a clean 12-line deletion, and gitlab:sidekiq:queues:check passes.
Worker removal and the rolling-deploy drain window
ExecuteMergeRequestReadyWorker was reduced to a no-op in 4b146d58ee33 on 2026-06-18 (19.1), with a comment saying it was "kept for one milestone for backward compatibility to drain in-flight Sidekiq jobs during rolling deploys". Three milestones have passed and nothing publishes to it - MergeRequests::ReadyEvent is consumed by Ai::Catalog::Flows::ExecuteMergeRequestReadyWorkflowTriggersWorker instead. Deleting the class now cannot orphan an in-flight job.
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:
- !242698 (merged) merges and its flag goes on. It adds the
merge_request createdtrigger, 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 behindmerge_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. dap_powered_recommend_reviewersis 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.- This MR merges, deleting what is by then already-dead code.
- !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).
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 theadditional_context_resolveron therecommend_reviewers/v1catalog item (ee/app/models/ai/catalog/foundational_flow/definitions/recommend_reviewers.rb), which is the trigger path.- The
recommend_reviewers/v1catalog item - it's the product now. - The
auto_assign_reviewersinternal event - still fired byAssignServicewithlabel: 'code_owners', and the label description stays accurate. - The
project_settings.reviewer_assignment_strategycolumn.
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
- The optional adjacent cleanup was not taken.
app/models/project_setting.rbcarriesignore_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:
- The worker reaches
AssignServiceand calls it for real (and_call_original), raises nothing, and assigns nobody. AssignServicereturns a success whose message is exactly'No strategy available'- pinning that it skips for want of a strategy rather than bailing earlier on some unrelatedskip_reasonsuch 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_ownerspath - No migration in this MR