:robot: This has been AI assisted with Duo Chat ## Summary Redesign the messaging adapter infrastructure introduced in !234959 / !232343 to provide a clean, scalable foundation for triggering AI flows from any messaging surface (Slack, GitLab note mentions, `@GitLabDuo`, future surfaces). The current design conflates orchestration with policy, blocks several upcoming use cases, and misses architectural feedback (`!234959#note_3328524508`). This issue captures the agreed-on end-state design and the reasoning that led to it. ## Context ### Where this came from The messaging adapter pattern was introduced for the Slack MVC (epic `&590434`): - !232332 — `WorkspaceProjectService` (merged) - !232343 — orchestrator, adapter base, callback infrastructure (merged) - !232441 — Slack adapter PoC (open) - !234788 — GitLab note adapter PoC (open) - !234959 — extracted messaging adapter infrastructure (in review, **to be closed in favor of this redesign**) - !235039 — GitlabNote mention adapter (stacked on !234959) - !235204 — flow versioning support (in review, parallel work) The PoCs surfaced bugs and architectural pain points (composite identity missing in Slack path, no authorization in messaging path, duplicated orchestration logic between `RunService` and `TriggerFlowService`). !234959 attempted to consolidate these. Review surfaced deeper problems that warrant a clean foundation rather than incremental fixes. ### Related work and decisions - ADR: `gitlab-com/content-sites/handbook!19714` — needs to be updated to reflect this design - Architectural discussion #599330 — `@GitLabDuo` mention vs Slack integration approaches; established that messaging adapter pattern + CI runner is the chosen direction - Flow versioning: !235204 — introduces `FoundationalFlow#flow_version`, `flow_config_id`, `flow_config_schema_version`; surfaces should be able to override version ### Surfaces that need to work | Surface | Auth model | Service account source | Flow source | Project source | |---------|------------|------------------------|-------------|----------------| | GitLab note `@mention` (foundational flows) | `:trigger_ai_flow` upstream + `FlowTrigger` record existence | From `FlowTrigger.service_account` | From the foundational flow tied to the consumer | `note.project` | | `@GitLabDuo` mention in MR | MR-level ability (e.g. `:execute_ai_review_merge_request`) | Dedicated bot service account | Today: borrows existing flow. Long-term: own catalog item that routes internally | MR's project | | Slack | Slack workspace install + namespace mapping | From namespace's catalog item consumer | Hardcoded `developer/v1` initially | Lazily-created `duo-workspace` project | | Future custom entry points (e.g. WhatsApp, webhooks) | Surface-defined | Surface-defined | Surface-defined | Surface-defined | Note on `@GitLabDuo`: today it already runs flows but bypasses the modern Duo Workflow path. The long-term plan is for `@GitLabDuo` to become a routing entry point — potentially with its own catalog item that internally dispatches to `developer/v1`, `code_review/v1`, or a future router agent. The redesign **must not assume** that the surface's catalog item and the executed flow's catalog item are the same. What the redesign locks in is the property that the surface owns auth and flow selection; it does not preclude `@GitLabDuo` later having its own first-class catalog item with its own quota/billing/audit attachment. ### Constraints to honor - Must not require Developer foundational flow to be enabled in a namespace just because `@GitLabDuo` borrows its current flow (kinsingh's UX problem in #599330) - Must support flow versioning per !235204 (surface can override version) - Must use composite identity primitives from `lib/gitlab/auth/identity.rb`, not parallel modules (Fred's blocking review on `!234959#note_3328524508`) - Non-foundational catalog flows are out of scope but must not be a one-way-door decision - Existing functionality (Code Review, Duo Developer assign/mention, Fix Pipeline) must keep working ## Problems with the current design (`!234959`) 1. **Orchestrator conflates orchestration with policy.** `TriggerFlowService` does namespace resolution, enablement check, service account resolution, authorization, project resolution, composite identity linking, SA membership grant, callback enrichment, and workflow execution. Too many responsibilities. 2. **Flow reference is overloaded.** A single `flow_reference` string drives four independent decisions: which `FoundationalFlow` to look up, which catalog item to authorize against, which catalog item to check enablement for, and which workflow definition to execute. This blocks any surface that wants to authorize differently than via namespace-level catalog item enablement (e.g. `@GitLabDuo`). 3. **Two parallel orchestration paths.** `RunService` (mention path, pre-resolved) and `TriggerFlowService` (Slack path, cold-start) both end up at `ExecuteWorkflowService` but resolve and gate completely differently. The promised "single orchestration primitive" doesn't exist. 4. **Composite identity duplicates auth-domain code.** `Ai::CompositeIdentity::Linker` recreates a pattern (`fabricate(sa).link!(user)`) that already exists in `Gitlab::Auth::Identity` as `link_from_web_request`, `link_from_oauth_token`, etc. Fred (review) and Sahasranamam (auth/composite identity owner) prefer the linking primitive lives in the auth domain. 5. **No room for flow version override.** !235204 introduces version pinning, but the orchestrator hardcodes version derivation from the catalog item consumer. A surface can't say "run developer/v1 at 1.0.0." 6. **Surface flexibility is poor.** Any new surface inherits the cold-start gating logic, even when it has its own auth path. Any divergence requires escape hatches. 7. **Blanket `safe { }` swallows errors that should fail loudly.** Wrapping every lifecycle hook in `rescue StandardError + track_exception` means that if `on_request_received` (which creates the user-visible ":arrows_counterclockwise: Processing..." note) fails, the user sees nothing yet the flow proceeds. The result is a workflow whose final response posts back into a discussion that never had an acknowledgement note. Resilience is good; silent resilience is not. 8. **Two failure hooks (`on_flow_failed`, `on_trigger_failed`) for what is really a single concern.** The dual hooks were introduced in !232441 to handle the Slack adapter logging spurious `no_reaction` errors when removing the :eyes: reaction in the sync failure path (where the reaction had never been added). The actual requirement is "adapters need to know whether earlier lifecycle state exists, so cleanup can be conditional," not two distinct hook names. ## Design decisions and reasoning ### Surface owns policy, base adapter owns mechanism The fundamental restructuring: the surface (concrete adapter) handles all decisions specific to its context — authorization, service account selection, flow selection, project selection, goal building. The shared infrastructure (`Adapters::Base`) handles only the mechanical work that no surface can or should do for itself — composite identity linking, SA project membership, callback context enrichment, resource translation, and workflow execution. This eliminates the "two parallel paths" problem for adapter-shaped surfaces because `RunService`-style pre-resolved surfaces and `TriggerFlowService`-style cold-start surfaces both fit the same model — they just do their resolution differently inside `build_trigger_bundle`. RunService itself remains a parallel path for non-foundational flows and the assignment-trigger path until a future consolidation moves the security primitives into ExecuteWorkflowService itself (out of scope, see "Out of scope"). ### Typed `TriggerBundle` value object instead of symbol-keyed hash `build_trigger_bundle` returns a `Data.define`-based value object, not a hash: ```ruby TriggerBundle = Data.define( :current_user, :service_account, :flow_reference, :flow_version, :project, :goal, :resource, :additional_context, :source_branch ) ``` `Data.define` raises `ArgumentError` on unknown keys and on missing required keys. This catches contract violations at construction time inside the adapter, instead of with a deep `NoMethodError` inside `ExecuteWorkflowService`. Adapter authors get attribute access (`bundle.service_account`) instead of `payload[:service_account]`. This pattern is established in the codebase, see for reference: - `ee/lib/security/scan_result_policies/push_settings_overrides.rb` - `ee/lib/gitlab/security/orchestration/pipeline_execution_policies/intervals.rb` - `lib/api/helpers/notes_helpers.rb` - `ee/app/services/search/zoekt/rollout_service.rb` ### No separate `TriggerFlowService` We considered keeping a thin `TriggerFlowService` between the adapter and `ExecuteWorkflowService` for symmetry. We rejected this because the service ends up being \~50 lines of glue around 3 lines of real work, with no behavior that isn't already part of the adapter's `trigger` lifecycle. A standalone service would be a fig leaf, not a meaningful boundary. The alternative of moving everything into concrete adapters was also rejected: composite identity linking and SA project membership are security-critical primitives that should not be per-adapter discretion. Putting them in `Adapters::Base#trigger` makes them inherited by contract. ### Composite identity preconditions split Three preconditions exist before linking today: 1. `service_account.composite_identity_enforced?` — auth-domain knowledge, lives in `Gitlab::Auth::Identity` 2. `Ai::Setting.instance.duo_workflow_oauth_application` is configured — AI-platform configuration check 3. `:ai_flow_triggers_use_composite_identity` feature flag — temporary rollout gate (1) goes into `Gitlab::Auth::Identity.link_for_ai_flow`. (2) and (3) are AI-flow concerns, kept as caller responsibilities in `Adapters::Base` (or removed when rollout completes). This addresses Fred's review without recreating `Ai::CompositeIdentity::Linker` inside `Gitlab::Auth::Identity` (which would miss the point). Fred's blocking review concern (`!234959#note_3328524508`) is satisfied by either this split or by pushing the FF + OAuth-app checks into `Gitlab::Auth::Identity.link_for_ai_flow` itself. We're going with the split above since the FF is an AI-flow rollout concern, not an auth-domain concern. Loop `@skundapur` in for review on the `link_for_ai_flow` addition (he owns `lib/gitlab/auth/identity.rb`); if he prefers everything in the auth domain, the change is a small refactor that doesn't affect the rest of this design. ### Tiered hook resilience (replaces blanket `safe { }`) Three categories of work in `Base#trigger`: 1. **Must succeed for the user to know anything happened.** Example: `on_request_received` creating the ":arrows_counterclockwise: Processing..." note. If this fails, the user has no acknowledgement that their request was received. The flow MUST NOT proceed silently. 2. **Should succeed but the world keeps spinning if it doesn't.** Example: `on_flow_started` updating the existing note from "Processing" to "Started." If this fails, the note still exists from step 1; worst case it shows "Processing" longer than it should. 3. **Hard requirements for correctness.** Example: composite identity linking, SA project membership grant, `ExecuteWorkflowService.execute`. These cannot be skipped, no wrapper. Implementation: ```ruby def must yield ServiceResponse.success rescue StandardError => e ::Gitlab::ErrorTracking.track_exception(e, adapter: self.class.name) ServiceResponse.error(message: e.message, reason: :hook_failed) end def safe yield rescue StandardError => e ::Gitlab::ErrorTracking.track_and_log_exception(e, adapter: self.class.name) end ``` `Base#trigger` short-circuits if `must { on_request_received }` returns an error. ### Single failure hook with workflow context We collapse `on_flow_failed` and `on_trigger_failed` into a single hook with an optional `workflow:` kwarg: ```ruby def on_flow_failed(callback_context:, error:, workflow: nil) deliver_error(callback_context: callback_context, error: error) end ``` - Sync path (`Base#trigger` after `ExecuteWorkflowService` returns error): pass `workflow: nil`. - Async path (`CallbackWorker` after a started workflow fails): pass the actual `workflow`. Adapters that need conditional cleanup branch on `workflow.present?`. The Slack adapter only removes the :eyes: reaction when `workflow.present?`, eliminating the phantom `no_reaction` log Marco flagged in `!232441#note_3317304974`. This is strictly more useful than two hook names because adapters get richer state (the workflow object itself, not just a phase boolean) and the abstraction matches natural language ("the flow failed"). ### Adapter registry built from descendants, not manual registration Each concrete adapter declares its own key as a class method: ```ruby class GitlabNote < Base ADAPTER_KEY = 'gitlab_note' def self.adapter_key = ADAPTER_KEY end ``` `CallbackWorker::ADAPTER_REGISTRY` is built from `Adapters::Base.descendants`, with a CI-time uniqueness check on `adapter_key`. `Base#enriched_callback_context` auto-injects `'adapter' => self.class.adapter_key` so adapters cannot accidentally write a string into the callback context that doesn't match their registry key. Removes a whole class of "I forgot to register / I wrote the wrong string" bugs. ### No `Flow` value object (yet) We initially proposed an `Ai::Messaging::Flow` struct to carry `reference`, `config_id`, `schema_version`, `version`. We dropped it: !235204 already does the splitting inside `StartWorkflowService` via `FoundationalFlow.for_reference`. The adapter only needs to pass `flow_reference` (string) and `flow_version` (string) to `ExecuteWorkflowService`. Avoids introducing a value object that would need to evolve as flow versioning matures. ### Always call `ServiceAccountMemberAddService` `ServiceAccountMemberAddService` short-circuits when membership exists. The cost is one indexed DB lookup. Always-on guarantees no surface ever forgets to grant access. The cost of forgetting (runtime permission failure deep in workflow execution) far exceeds the cost of one extra query. ## Proposed architecture ``` Surface event (Slack message, MR note, webhook, ...) │ ▼ Concrete Adapter (extends Adapters::Base) │ - knows the surface │ - does its own auth │ - resolves: user, service_account, flow_reference, flow_version, project, resource, goal │ - implements build_trigger_bundle + build_callback_context + delivery │ ▼ adapter.trigger Adapters::Base#trigger │ - must { on_request_received } -- short-circuits on failure │ - build_trigger_bundle (returns TriggerBundle or error) │ - build_callback_context -- runs after on_request_received │ - composite identity link (via Gitlab::Auth::Identity) │ - service account project membership (via ServiceAccountMemberAddService) │ - callback context enrichment (service_account_id, flow_reference, flow_version, adapter_key) │ - resource translation (Issue → issue_id, MergeRequest → merge_request_id) │ ▼ Ai::Catalog::ExecuteWorkflowService.execute (existing, unchanged) │ ▼ Workflow runs on CI runner │ ▼ async, on completion Ai::Messaging::CallbackWorker │ - looks up adapter from descendants registry │ - dispatches deliver_result / on_flow_completed / on_flow_failed (with workflow:) ▼ Adapter posts result back to surface ``` ### Components to add #### `Gitlab::Auth::Identity.link_for_ai_flow` (new method on existing class) ```ruby # in lib/gitlab/auth/identity.rb def self.link_for_ai_flow(service_account:, scoped_user:) return unless service_account return unless scoped_user return unless service_account.composite_identity_enforced? link_from_web_request(service_account: service_account, scoped_user: scoped_user) end ``` Lives alongside existing factory methods (`link_from_web_request`, `link_from_oauth_token`, etc.). Owner: `@skundapur` and the Auth team. #### `Ai::Messaging::Adapters::Base` (redesigned) ```ruby module Ai module Messaging module Adapters class Base # === Sync construction (called from #trigger) === # Concrete adapters call super(...) with full constructor state. # === Async construction (called from CallbackWorker) === # Subclass implements from_callback_context. Returned instance # has access to callback_context only -- constructor attrs like # current_user, project, etc. are nil. def self.from_callback_context(_ctx) = new def self.adapter_key = raise NotImplementedError # Wraps the lifecycle: ack -> resolve -> link -> grant -> execute -> notify. # `on_request_received` runs BEFORE `build_callback_context` so adapters # can stash state (e.g. progress note) used during context serialization. def trigger ack = must { on_request_received } return ack if ack.error? bundle = build_trigger_bundle ctx = build_callback_context unless bundle.success? safe { on_flow_failed(callback_context: ctx, error: bundle.reason, workflow: nil) } return bundle end payload = bundle.payload # TriggerBundle instance if composite_identity_eligible?(payload.current_user) ::Gitlab::Auth::Identity.link_for_ai_flow( service_account: payload.service_account, scoped_user: payload.current_user ) end member_result = ::Ai::ServiceAccountMemberAddService .new(payload.project, payload.service_account).execute unless member_result.success? safe { on_flow_failed(callback_context: ctx, error: :service_account_error, workflow: nil) } return ServiceResponse.error(message: member_result.message, reason: :service_account_error) end result = ::Ai::Catalog::ExecuteWorkflowService.new( payload.current_user, container: payload.project, goal: payload.goal, service_account: payload.service_account, flow_definition: payload.flow_reference, item_version: payload.flow_version, source_branch: payload.source_branch || payload.project.default_branch_or_main, additional_context: payload.additional_context, messaging_callback_context: enriched_callback_context(ctx, payload), **resource_params(payload.resource) ).execute if result.success? safe { on_flow_started(callback_context: ctx, workflow: result.payload[:workflow]) } else safe { on_flow_failed(callback_context: ctx, error: :execute_workflow_failed, workflow: nil) } end result end # === Surface contract — concrete adapter MUST implement === TriggerBundle = Data.define( :current_user, :service_account, :flow_reference, :flow_version, :project, :goal, :resource, :additional_context, :source_branch ) # Returns ServiceResponse.success(payload: TriggerBundle.new(...)) # OR ServiceResponse.error(message:, reason:) def build_trigger_bundle = raise NotImplementedError def build_callback_context = raise NotImplementedError def deliver_result(callback_context:, message:) = raise NotImplementedError def deliver_error(callback_context:, error:) = raise NotImplementedError # === Lifecycle hooks === # Override as needed. Defaults are no-ops or delegate to deliver_error. # Sync, called from #trigger before resolving the bundle. # MUST raise on failure -- the framework treats this as a hard precondition. def on_request_received; end # Sync, called from #trigger after ExecuteWorkflowService succeeds. def on_flow_started(callback_context:, workflow:); end # Async, called from CallbackWorker on success. def on_flow_completed(callback_context:, workflow:); end # Called from BOTH paths: # - Sync (Base#trigger): workflow: nil # - Async (CallbackWorker): workflow: <Workflow instance> # Adapters that need conditional cleanup branch on workflow.present?. def on_flow_failed(callback_context:, error:, workflow: nil) deliver_error(callback_context: callback_context, error: error) end private def composite_identity_eligible?(user) return false unless user return false unless Feature.enabled?(:ai_flow_triggers_use_composite_identity, user) return false if ::Ai::Setting.instance.duo_workflow_oauth_application.nil? true end def resource_params(resource) case resource when Issue then { issue_id: resource.iid } when MergeRequest then { merge_request_id: resource.iid } else {} end end def enriched_callback_context(ctx, payload) ctx.merge( 'adapter' => self.class.adapter_key, 'service_account_id' => payload.service_account.id, 'flow_reference' => payload.flow_reference, 'flow_version' => payload.flow_version ).compact end def must yield ServiceResponse.success rescue StandardError => e ::Gitlab::ErrorTracking.track_exception(e, adapter: self.class.name) ServiceResponse.error(message: e.message, reason: :hook_failed) end def safe yield rescue StandardError => e ::Gitlab::ErrorTracking.track_and_log_exception(e, adapter: self.class.name) end end end end end ``` #### `Ai::Messaging::CallbackWorker` (descendants-based registry) - Registry built from `Adapters::Base.descendants`, indexed by `adapter_key` - CI-time uniqueness check on `adapter_key` across all subclasses - Resolves adapter via `from_callback_context` - Dispatches `deliver_result` / `on_flow_completed` / `on_flow_failed(workflow: workflow)` based on workflow status - Per-hook `safe { }` isolation so a failing `on_flow_completed` doesn't prevent `deliver_result` from running ### Components to remove - `Ai::Messaging::TriggerFlowService` (introduced in !232343, currently on master) — deleted; logic absorbed into `Adapters::Base` Note: `Ai::CompositeIdentity::Linker` was introduced in !234959 and does not exist on master. Since !234959 is being closed without merge and this issue starts fresh from master, there is nothing to delete — `Linker` is simply never added. ### Surface contract | Concern | Responsibility | |---------|----------------| | Receive surface event | Adapter constructor | | Authorize the user | Inside `build_trigger_bundle` (any ability check, surface-specific policy) | | Pick the service account | Inside `build_trigger_bundle` | | Pick the flow (`flow_reference`, `flow_version`) | Inside `build_trigger_bundle` | | Pick the project | Inside `build_trigger_bundle` | | Build the goal | Inside `build_trigger_bundle` | | Build callback context | `build_callback_context` (runs after `on_request_received`) | | Deliver result/error to surface | `deliver_result`, `deliver_error` | | Lifecycle UX (e.g. progress note) | Override `on_request_received` (must succeed) and `on_flow_started`, `on_flow_failed` (best effort) | | Composite identity link | **Never** — Base handles it | | SA project membership | **Never** — Base handles it | | Resource translation | **Never** — Base handles it | | Adapter key in callback context | **Never** — Base injects from `self.class.adapter_key` | | Flow enablement check | **Never** — surface decides during auth if relevant | ### Surface examples (informative, not part of this issue) **GitLab note mention adapter (foundational flows)**: authorizes via `:trigger_ai_flow` upstream; resolves SA + flow from a pre-existing `FlowTrigger` record; project = `note.project`; resource = `note.noteable`. `on_request_received` creates the progress note; `build_callback_context` (runs after) embeds `progress_note_id` into the context. `on_flow_failed` updates the progress note in place when `workflow.present?` (async failure) or when the progress note exists (sync failure). **`@GitLabDuo` MR mention adapter**: authorizes via MR-level ability; uses dedicated bot service account; flow + version from whatever flow it borrows or routes to. Does **not** require the borrowed flow to be enabled in the namespace because authorization is independent of catalog enablement. Long-term, may grow its own catalog item that internally dispatches. **Slack adapter**: authorizes via Slack workspace install + namespace mapping; resolves SA from namespace's catalog item consumer; project = lazily-created workspace project via `WorkspaceProjectService`; flow hardcoded to `developer/v1` initially. `on_flow_started` adds :eyes:; `on_flow_failed` removes :eyes: only if `workflow.present?`, then adds :x:. **Custom future entry point**: any auth model, any SA, any flow + version. ## Properties this gives us 1. **Composability for adapter-shaped surfaces**: New adapter-shaped surface = new adapter. No changes to `Base`, `ExecuteWorkflowService`, or anything else. 2. **No redundant gating**: Each surface gates exactly once, in its own auth path. The shared infrastructure never re-validates. 3. **Versioning-ready**: Surface picks `flow_version`. Aligns with !235204. 4. **No model coupling**: `flow_reference` and `flow_version` are strings. 5. **No one-way doors for non-foundational flows or `@GitLabDuo` evolution**: Future surface could derive `flow_reference` and `flow_version` (or leave version `nil`) from a non-foundational catalog item; `ExecuteWorkflowService` already handles both. 6. **Security primitives centralized for adapter-shaped surfaces**: Composite identity link and SA membership are guaranteed by Base#trigger. RunService (non-foundational mention path, assignment path) remains a parallel flow until a future consolidation moves these primitives into ExecuteWorkflowService itself (out of scope). 7. **Tiered resilience**: Hard preconditions raise; UX-polish hooks are best-effort with full error tracking. No silent acknowledgement failures. 8. **Auth-domain alignment**: Composite identity linking lives in `Gitlab::Auth::Identity` per Fred's review. 9. **Type-checked adapter contract**: `TriggerBundle` (Data.define) catches missing keys at adapter-construction time, not deep in `ExecuteWorkflowService`. ## Scope of this issue This issue captures one piece of work: redesigning the messaging adapter base from current master. !234959 will be closed without merge; we start fresh. Concretely, one MR delivers: - Add `Gitlab::Auth::Identity.link_for_ai_flow` (review by `@skundapur` since he owns the file) - Redesigned `Ai::Messaging::Adapters::Base`: `TriggerBundle` value object, tiered resilience (`must` + `safe`), single failure hook with `workflow:` kwarg, `adapter_key` class method, auto-injection into callback context - `Ai::Messaging::CallbackWorker` updated: descendants-based registry, per-hook `safe { }` isolation - Delete `Ai::Messaging::TriggerFlowService` (introduced in !232343, currently on master) - Refactor `Ai::FlowTriggers::RunService` to use `link_for_ai_flow` directly (replaces the inline `fabricate(sa).link!(user)` pattern that exists on master). If this pushes the diff size beyond reviewable scope, defer to a follow-up — it does not affect the adapter pattern. The ADR `gitlab-com/content-sites/handbook!19714` will be updated in a separate MR to reflect this design. ### Out of scope (tracked separately) - Rebasing !235039 (GitlabNote adapter) on the new Base. Stacked work, lands after this. - Slack adapter (!232441) and `@GitLabDuo` MR mention adapter — separate efforts in their own MRs. - Long-term consolidation: moving composite identity link + SA membership into `ExecuteWorkflowService` itself. This is the strictly cleaner end-state (eliminates `RunService` as a parallel path entirely) but is a substantive separate effort. Worth a follow-up issue. ## Implementation notes Things the implementer needs to know but that don't gate implementation. 1. **Hook ordering contract**: `on_request_received` runs before `build_callback_context` so adapters like GitlabNote can stash state (e.g. `@progress_note`) used during context serialization. Document this contract loudly in `Base#trigger` so it is not accidentally inverted in a future refactor. 2. **Async vs sync method visibility**: methods on a concrete adapter run in two contexts — sync via `#trigger` (full constructor state available) and async via `from_callback_context` (only callback_context hash available). Document which constructor attributes are nil in the async path. Group methods with comment dividers (`# === Sync ===` / `# === Async ===`) so adapter authors don't accidentally call sync-only state from async hooks. 3. **Flow version precedence**: !235204's `StartWorkflowService` resolves flow version with feature-flag overrides (`duo_developer_next_unstable`, `fix_pipeline_next`). The precedence is: adapter `flow_version` arg \> FF override replaces it for unstable variants. Document this in `Adapters::Base` so adapter authors are not surprised that `flow_version: '2.0.0'` becomes `'1.0.0'` when `duo_developer_next_unstable` is set. 4. **Spec isolation**: !234959's `trigger_flow_service_spec.rb` had a CI flake (12 tests failing on first run, all passing on retry — likely test ordering issue from spec restructuring). When writing the new `Base` spec, prefer `let` over `let_it_be` for state stubbed via `stub_feature_flags`; isolate flow-enablement contexts in their own `before` blocks. 5. **Test coverage**: follow standard GitLab contribution requirements and the patterns used by maintainers in this area. Reference !234959's spec structure for context-coverage breadth (auth, namespace resolution, project resolution, FF-on/off, error reasons), but write the spec fresh against the new `Base`. 6. **Rollback / migration considerations**: messaging adapter is not yet used in production by anyone (Slack adapter is internal MVC, GitlabNote adapter is unmerged PoC). No production callback-context rows to migrate; no rollback FF needed. 7. **What carries over from !234959**: !234959 contained genuine bug fixes alongside the architectural mistakes. When implementing fresh, preserve these: - `:execute_ai_catalog_item` authorization check on the messaging path - `service_account_id` and `flow_reference` enrichment of `callback_context` - `pinned_version_prefix` resolution from the item consumer - `service_account_error` / `unauthorized` / `workspace_project_error` reason symbols and their `log_and_error` pattern - The `resolve_service_account_and_consumer` consolidation (passes the project as container so the child consumer is found, not the group-level parent) - `Resolve service account service` payload includes `item_consumer` (the `ResolveServiceAccountService` change in !234959) These pre-date the architectural concerns and should not be lost. ## Known limitations to address before rollout - **Epic noteables**: `note.project` is nil for epic notes, so the GitlabNote adapter cannot post a reply. Today this means epic mentions silently fail. Add an explicit guard at the post-process layer that rejects epic-noteable triggers with a clear error, until a separate group-scoped delivery path exists. - **Conversation context performance**: GitlabNote adapter loads all discussion notes via `Note.where(discussion_id:).user.includes(:author)` before slicing the most recent N. For a 500-note discussion this is a moderately expensive query. Verify with a load test before rolling out widely, or add a SQL `LIMIT` upstream. ## Migration / cleanup - Close !234959 without merge (referencing this issue) - Rebase !235039 (GitlabNote adapter) on the new Base once this lands - Update ADR `gitlab-com/content-sites/handbook!19714` in a separate MR to reflect this design - Loop `@skundapur` in for review on the `Gitlab::Auth::Identity.link_for_ai_flow` addition ## References - Epic: `&590434` - Architecture decision: #599330 - ADR (pending update): `gitlab-com/content-sites/handbook!19714` - Predecessor MRs: !232332, !232343, !232441, !234788, !234959, !235039, !235204 - Review concern that motivated auth-domain alignment: `!234959#note_3328524508` (Fred de Gier) - Review concern that motivated single failure hook: `!232441#note_3317304974` (Marco Gregorius) - `Gitlab::Auth::Identity` source: `lib/gitlab/auth/identity.rb`