Resolve the Duo Code Review actor for a composite requester

Denys Mishunovrequested to merge
612038-duo-code-review-actor-scoped-user-override into master

What does this MR do and why?

A merge request that asks GitLab Duo for a second review round gets no review at all: no workflow, no job, no note, no error. Only the first round works.

EE::MergeRequests::BaseService#duo_code_review_actor returns nil for the requester, and start_duo_code_review then returns before enqueuing StartReviewWorker. Both exits are log-only, so nothing surfaces to the requester.

Root cause

!249624 (merged) introduced EE::MergeRequests::BaseService#duo_code_review_actor. The out-of-band start needed to resolve a billable human at enqueue time, and that MR guarded that resolution on identity.composite?, which is request state wearing a user-shaped name. A composite request arrives as the scoped human carrying an in-memory override, so it answered composite and unlinked, and the actor came back nil. It only shows on a bare reviewer change, because the un-draft path passes a freshly loaded author with no override, which is why round one always worked and every flow re-request silently did not. Exposure is limited to where duo_code_review_clean_identity_start is enabled.

A composite-identity request does not arrive as the service account. Gitlab::Auth::AuthFinders#resolve_composite_identity_user makes current_user the scoped user and stamps an in-memory composite_identity_enforced! override onto it:

identity.scoped_user.composite_identity_enforced!
identity.scoped_user

So an ordinary human answers true to composite_identity_enforced? for the rest of that request, because that reader returns the override before the column. The guard this MR replaces then asked Identity#composite?, which reads exactly that, and Identity#linked?, which reads user:<id>:composite_identity, keyed on the primary, so it is false for that same human. Composite and unlinked at once resolved to no actor.

Why only some requests

assign_duo_as_reviewer_automatically sets duo_review_user to the author, and it runs only on create and on the draft-to-ready transition (UpdateService#handle_draft_state_change). Every other request reaches request_duo_code_review with nothing set, so its user = duo_review_user || current_user falls back to current_user. That is the line the whole bug turns on:

Entry point user passed Result
Un-draft merge_request.author, freshly loaded, no override works
Bare reviewer change current_user, carrying the override dropped

Setting the reviewer is exactly how an automated flow asks for another round, so this dropped every re-request from a flow while leaving round one healthy, which is what made it hard to see.

The fix

Guard the composite branch on User#ai_service_account? (service_account? and composite_identity_enforced?) instead of the override alone. The column can only be true for a service account per the User validation, so a non-service-account answering true is carrying the request-scoped override and is already the resolved scoped user. Returning it unchanged is the correct answer, not a fallback, and User#authorization_user already guards the same lookup the same way.

The deliberate refusal for a genuine composite service account with no identity of its own linked is unchanged: entitlement there was resolved through another flow's identity, so there is no human to bill and the worker would be handed an account that can hold no add-on seat.

How this was confirmed

Reproduced twice on gitlab.com (in the dap-sw-factory project), then confirmed from production logs. The failing request carried:

Duo Code Review flow not started: no billable human for the requesting identity

Screenshot_2026-09-01_at_09.14.30

That message appears exactly once across the whole run window, and is absent from both the un-draft and a human-issued control on the same merge request minutes apart:

Time (UTC) Requester Log line Review
22:35:15 un-draft, author is a human none started
22:41:03 bare reviewer change, composite override present none
22:48:15 bare reviewer change, human PAT none started in 4s

How to set up and validate locally

Three paths, in increasing setup cost. Pick the cheapest one that answers your question.

Path Proves Needs
1. Actor resolution, in a console the defect itself, in one method call a GDK, nothing else
2. The full enqueue path that the dropped actor is what suppresses the review a Duo-enabled project
3. End to end, from a flow the original report, with a real composite token the above, plus a pasted AI Catalog flow

Pro tip: you can guide your agents to this section and ask it to follow them literally to ensure all three paths work exactly as needed.

All three are before/after comparisons. To switch sides:

git fetch origin 612038-duo-code-review-actor-scoped-user-override
git checkout 612038-duo-code-review-actor-scoped-user-override   # or: git checkout master

gdk restart rails-web rails-background-jobs

Then open a fresh rails c. The guard runs in web and the enqueue lands in Sidekiq, so both have to be on the same code, and a console started before the switch is stale.

1. The actor resolution, in a rails console

This is the whole defect and it needs no Duo setup, no service account, no feature flag and no merge request. composite_identity_enforced! is an in-memory setter, so calling it is exactly what AuthFinders does to the scoped user on a composite request.

project = Project.find_by_full_path('gitlab-duo/test')   # any project will do
human   = User.find_by_username('root')

human.composite_identity_enforced!   # what AuthFinders stamps onto the scoped user

service = MergeRequests::UpdateService.new(project: project, current_user: human, params: {})
service.send(:duo_code_review_actor, human)
Result Meaning
master nil no actor, so start_duo_code_review returns before enqueuing anything
this branch #<User id:1 @root> the human is returned and StartReviewWorker is enqueued

send is used because the method is private; there is no public seam that isolates it.

2. The full enqueue path, on a Duo-enabled project

Setup. On a stock GDK the project is gitlab-duo/test, which gdk seeds inside the gitlab-duo group that carries the Duo add-on. Any project in a Duo-enabled group works.

  1. Settings -> General -> GitLab Duo (/-/settings/general). Turn on, in this order, because each gates the next in the UI: GitLab Duo, Remote GitLab Duo Flows, Foundational GitLab Duo Flows.
  2. Automate -> Flows (/-/automate/flows), enable Code Review. This also provisions a composite service account with Developer access on the project and composite_identity_enforced: true, so there is nothing to create by hand.
  3. Give the requester a Duo seat, on the group level at Settings -> GitLab Duo -> Seat utilization (/-/settings/gitlab_duo/seat_utilization).
  4. Have at least one open merge request that duo_code_review_startable? accepts: open, with its diffs persisted and non-empty. Any one will do; the snippet picks the first that qualifies and prints which.

Check the setup. In rails c:

project = Project.find_by_full_path('gitlab-duo/test')
human   = User.find_by_username('root')

Feature.enable(:duo_code_review_clean_identity_start, project)

Feature.enabled?(:duo_code_review_clean_identity_start, project)  # => true
project.duo_code_review_dap_available?                            # => true, else setup 1 or 2 is incomplete
Gitlab::Duo::CodeReview.dap?(user: human, container: project)     # => true, else `human` has no Duo seat

That last one is Gitlab::Duo::CodeReview.dap?, which dispatches to Modes::Dap#active?. It needs both Ai::DuoAgentPlatform.available? for the user and duo_code_review_dap_available? on the project, which is what setup steps 1 and 2 turn on.

Run it, in the same console:

sa = User.where(user_type: :service_account, composite_identity_enforced: true)
         .find { |u| project.team.member?(u) }
raise 'No composite service account: do setup step 2.' unless sa

duo = Users::Internal.in_organization(project.organization_id).duo_code_review_bot
mr  = project.merge_requests.opened.find do |m|
  m.duo_code_review_startable? && !m.duo_code_review_in_flight?
end
raise 'No suitable merge request: do setup step 4, or wait for a running review to finish.' unless mr

puts "service account = #{sa.username} / target = !#{mr.iid}"
before = Ai::DuoWorkflows::Workflow.where(project_id: project.id).maximum(:id).to_i

# Outside a request store the identity link is not stored anywhere, `linked?` is false for a
# different reason than the one under test, and the scenario silently is not the one you meant.
Gitlab::SafeRequestStore.ensure_request_store do
  # What `resolve_composite_identity_user` leaves behind on a composite request: the link is
  # stored under the SERVICE ACCOUNT, the override is stamped on the scoped human.
  Gitlab::Auth::Identity.fabricate(sa).link!(human, context: :authentication)
  human.composite_identity_enforced!

  svc = ->(params) { MergeRequests::UpdateService.new(project: project, current_user: human, params: params) }
  svc.call(reviewer_ids: []).execute(mr)          # clear, then set: see below
  svc.call(reviewer_ids: [duo.id]).execute(mr)
  puts "reviewers -> #{mr.reload.reviewer_ids.inspect}"
end

90.times do
  break if Ai::DuoWorkflows::Workflow.where(project_id: project.id).where('id > ?', before).exists?
  sleep 1
end
wf = Ai::DuoWorkflows::Workflow.where(project_id: project.id).where('id > ?', before).first
puts wf ? "workflow #{wf.id} #{wf.workflow_definition} CREATED -> fix in effect" \
        : 'NO workflow created -> bug reproduced'
last line
master NO workflow created -> bug reproduced. grep 'no billable human' log/application_json.log in the GDK confirms it from the other side (there is no plain application.log).
this branch workflow <id> code_review/v1 CREATED -> fix in effect

Three things that each turn into a false negative:

  • The clear-then-set is load-bearing. handle_reviewers_change, the single entry point for a reviewer change, fires on a change, so re-setting a reviewer that is already set does nothing at all. It is also what makes the snippet repeatable, which you need in order to run it on both branches.

  • The service account it prints may not be duo-code-review-<group>. Any composite service account with Developer access on the project does, since it only stands in as the primary of the identity link. Developer is the part that matters, because Ability#with_composite_identity_check re-runs the permission check as the primary; without it the reviewer write is refused and the failure looks exactly like the bug.

  • A successful run starts a real review, so let it finish before running the other branch:

    mr.reload.merge_request_reviewers.map { |r| [r.reviewer.username, r.state] }
# => [["GitLabDuo", "reviewed"]]

3. End to end, with a flow you can paste into a GDK

A composite identity comes from the token, so it cannot be built by hand with a PAT: resolve_composite_identity_user only runs for a token whose user has composite_identity_enforced. A human issuing the identical REST call takes the non-composite path and succeeds, which is precisely why this went unnoticed, and why reproducing the original report needs a flow. The definition below is a minimal one, so this is checkable without any particular flow of your own. It creates nothing and pushes nothing.

Setup. The same project as path 2, plus:

  1. Take one open merge request with a real diff and set GitLab Duo as its reviewer by hand. It reviews normally. That is the human control, and the one path that always worked.

  2. Wait for that review to finish, otherwise duo_code_review_in_flight? blocks the next one and you get a false negative. In rails c:

    mr.merge_request_reviewers.map { |r| [r.reviewer.username, r.state] }
# => [["GitLabDuo", "reviewed"]]

  3. Explore -> AI Catalog -> Flows -> New flow (/explore/ai-catalog/flows/new). Point Managed by at the project, give it any name and description, and paste the definition below into YAML configuration.

Flow definition (paste into an AI Catalog flow item) 
# PROBE / DEMO: does a flow-issued bare reviewer change start a Duo Code Review?
#
# Minimal reproduction of the actor drop in
# https://gitlab.com/gitlab-org/gitlab/-/work_items/612038, written to be pasteable into a
# GDK's AI Catalog by someone with none of the surrounding context. It creates nothing and
# pushes nothing: it re-requests review on a merge request that ALREADY has GitLab Duo as a
# reviewer, which is the single action that reproduces the bug.
#
# PRECONDITION: one open merge request in the target project, with a real diff, with GitLab Duo
# already set as a reviewer. Setting that by hand is also the human control -- it succeeds.
#
# WHY IT HAS TO BE A FLOW. A composite identity comes from the token, so it cannot be built by
# hand with a PAT: `resolve_composite_identity_user` only runs for a token whose user has
# `composite_identity_enforced`. A human issuing the identical REST call takes the
# non-composite path and succeeds, which is why this went unnoticed.
#
# HOW TO READ IT. The last line is the answer:
#   VERDICT=BUG_REPRODUCED   -> reviewer stayed `unreviewed`; no review was started.
#   VERDICT=REVIEW_STARTED   -> reviewer reached `review_started`; the fix is in effect.
#
# Prints bodies, never exit codes: `glab` exits 0 on an API error body.
#
# VERIFIED as a matched pair on a GDK (project gitlab-duo/test, MR !5, same flow, same run
# shape), with only the guard in EE::MergeRequests::BaseService#duo_code_review_actor differing:
#   master guard  -> reviewer_state_after=unreviewed      VERDICT=BUG_REPRODUCED
#   with the fix  -> reviewer_state_after=review_started  VERDICT=REVIEW_STARTED
version: "v1"
environment: ambient

components:

  - name: "rerequest"
    type: DeterministicStepComponent
    tool_name: "run_command"
    inputs:
      - from: >-
          echo "=====DCR_PROBE_BEGIN=====";
          echo "glab=$(glab --version 2>&1 | head -1)";
          echo "python3=$(python3 --version 2>&1 | head -1)";
          echo "project_path=${GITLAB_PROJECT_PATH:-UNSET}";
          echo "-- identity this flow presents (a composite token reports the SCOPED HUMAN) --";
          glab api user 2>&1 | python3 -c 'import json,sys; d=json.load(sys.stdin); print("  glab api user -> " + str(d.get("username")) + " id " + str(d.get("id")))' 2>&1;
          P=$(python3 -c 'import urllib.parse,os; print(urllib.parse.quote(os.environ.get("GITLAB_PROJECT_PATH",""), safe=""))' 2>&1);
          echo "-- target: an open merge request that already has a Duo reviewer --";
          glab api "projects/$P/merge_requests?state=opened&per_page=50" > /tmp/mrs.json 2>&1;
          python3 -c 'import json; rows=json.load(open("/tmp/mrs.json")); rows=rows if isinstance(rows,list) else []; hit=next((m for m in rows if any("duo" in (r.get("username") or "").lower() for r in (m.get("reviewers") or []))), None); print("NONE 0") if hit is None else print(str(hit["iid"]) + " " + str(next(r["id"] for r in hit["reviewers"] if "duo" in r["username"].lower())))' > /tmp/target 2>&1;
          cat /tmp/target;
          IID=$(cut -d" " -f1 /tmp/target 2>/dev/null); DUO=$(cut -d" " -f2 /tmp/target 2>/dev/null);
          test "$IID" = "NONE" && { echo "PROBE_PRECONDITION_UNMET: no open merge request has a Duo reviewer -- set one by hand, then re-run"; echo "=====DCR_PROBE_END====="; exit 0; };
          echo "  target=!$IID duo_id=$DUO";
          echo "-- reviewer state BEFORE --";
          glab api "projects/$P/merge_requests/$IID/reviewers" 2>&1 | python3 -c 'import json,sys; print("  " + "/".join(str(r.get("state")) for r in json.load(sys.stdin)))' 2>&1;
          echo "-- the bare clear-then-set, issued BY THE FLOW --";
          glab api -X PUT "projects/$P/merge_requests/$IID" -f reviewer_ids=0 2>&1 | python3 -c 'import json,sys; print("  cleared -> " + str([r["username"] for r in (json.load(sys.stdin).get("reviewers") or [])]))' 2>&1;
          glab api -X PUT "projects/$P/merge_requests/$IID" -f "reviewer_ids=$DUO" 2>&1 | python3 -c 'import json,sys; print("  set     -> " + str([r["username"] for r in (json.load(sys.stdin).get("reviewers") or [])]))' 2>&1;
          echo "-- waiting 90s for a review to start --";
          sleep 90;
          echo "-- reviewer state AFTER --";
          glab api "projects/$P/merge_requests/$IID/reviewers" 2>&1 | python3 -c 'import json,sys; s="/".join(str(r.get("state")) for r in json.load(sys.stdin)); print("  reviewer_state_after=" + s); print("  VERDICT=" + ("BUG_REPRODUCED" if s == "unreviewed" else "REVIEW_STARTED"))' 2>&1;
          echo "=====DCR_PROBE_END=====";
        as: "command"
        literal: true
    ui_log_events:
      - "on_tool_execution_success"
      - "on_tool_execution_failed"

  - name: "report"
    type: AgentComponent
    prompt_id: "dcr_probe_report"
    model_size_preference: "small"
    inputs:
      - from: "context:rerequest.tool_responses"
        as: "shell_out"
    toolset: []
    ui_log_events:
      - "on_agent_final_answer"

routers:
  - from: "rerequest"
    to: "report"
  - from: "report"
    to: "end"

flow:
  entry_point: "rerequest"

prompts:
  - prompt_id: "dcr_probe_report"
    name: "DCR composite re-request probe report"
    unit_primitives:
      - duo_agent_platform
    params:
      timeout: 300
    prompt_template:
      system: |
        You are a transcription step in a diagnostic probe. You have no tools and nothing to
        decide. Reproduce what you are given, exactly, and add nothing.

        Output the shell output verbatim, from `=====DCR_PROBE_BEGIN=====` to
        `=====DCR_PROBE_END=====` inclusive, and nothing else.

        Do not summarise, do not interpret, do not fix anything that looks broken, and do not
        omit a line because it looks like noise. A probe you have tidied up is a probe that has
        to be run again.
      user: |
        Shell output:
        {{ shell_out }}

        Transcribe.

  1. On the new flow's page, Enable flow, and pick the same project.

  2. Start it. There is no run button for a custom flow anywhere in the UI, so this goes over the API. With GITLAB_HOST=gdk.test:8080 (assuming you run GDK on gdk.test:8080) and a PAT for root in GITLAB_TOKEN:

    # the consumer id that step 4 created, which the UI never shows
glab api graphql -f 'query={ aiCatalogConfiguredItems(projectId: "gid://gitlab/Project/<project-id>", itemTypes: [FLOW]) { nodes { id item { name } } } }'

# start_workflow=true is mandatory: without it the endpoint returns the resolved
# YAML with a 200 and starts nothing
glab api -X POST ai/duo_workflows/workflows \
  -F project_id=<project-id> -F ai_catalog_item_consumer_id=<consumer-id> \
  -F start_workflow=true -f goal='probe'

  3. Read it at Automate -> Agent sessions (/-/automate/agent-sessions/<workflow-id>, the id the POST returned). Allow about three minutes: the flow itself sleeps 90 seconds waiting for a review to start. Its last step transcribes the shell output verbatim, so the verdict is in the final answer.

Result. The last line of the run is the answer:

reviewer_state_after verdict
master unreviewed VERDICT=BUG_REPRODUCED, nothing was started
this branch review_started VERDICT=REVIEW_STARTED

Verified exactly that way: same GDK, same project, same merge request, same flow, only the guard in duo_code_review_actor differing between the two runs.

The run also prints glab api user -> root, i.e. the flow reports as the scoped human from inside the job. That is the composite identity seen from the inside, and it is why the existing log line names a human rather than the service account.

Related

Related to https://gitlab.com/gitlab-org/gitlab/-/work_items/612038 — a defect in the out-of-band start that issue drove, behind the same duo_code_review_clean_identity_start flag, so the exposure is limited to where that flag is enabled.

MR acceptance checklist

  • Tests added for the regression, and verified to fail without the fix
  • Existing behaviour for a genuine composite service account preserved and covered
  • rubocop clean on changed files
  • Behind an existing feature flag, no new flag needed
  • Reviewer to confirm whether group::code review should own this, given it touches composite-identity resolution
Edited by Denys Mishunov

Merge request reports