DEV Community

Pavan Kumar Sadashiv
Pavan Kumar Sadashiv

Posted on Edited on

Part 1: Taming GitLab's EE::Project God Object — Extracting Ai::Catalog and Dodging the let_it_be Trap

#gitlab #ruby #rails #architecture

Series Context: This 10-part series documents real architectural refactorings, Model Context Protocol (MCP) server enhancements, AppSec fixes, and CI/CD review lessons shipped in gitlab-org/gitlab.

Featured MR: gitlab-org/gitlab!269060 (Merged into master) · Work Item: #636676

Why Project and Group Keep Growing — And How GitLab Stops Them

In a mature Rails monolith the size of GitLab, adding a helper method onto Project or EE::Project feels deceptively convenient. Project already holds associations, settings, and feature flags (duo_features_enabled, project_setting, license checks), so putting a domain predicate like project.ai_catalog_available? right there saves a class lookup today—at the cost of coupling every future AI Catalog change to GitLab's largest god object.

To stop this accretion, GitLab documents a clear policy under Taming Omniscient classes (doc/development/software_design.md) and enforces it in CI via the custom RuboCop cop Gitlab/OmniscientClassMethods.

How the cop works:

  1. It watches Project, Group, User, Organizations::Organization, and their EE::* extensions.
  2. Each protected class has two YAML manifests under rubocop/cop/gitlab/config/omniscient_classes/<class_name>/:
    • intrinsic_methods.yml: Identity methods strongly tied to the database record (e.g., full_path).
    • legacy_methods.yml: Grandfathered methods slated to move into their own bounded contexts. (ee_project/legacy_methods.yml still carried ~187 lines; project/legacy_methods.yml over 450.)
  3. Crucially, OmniscientClassMethods#on_investigation_end computes: 
   stale = self.class.baseline[class_name] - @defined_methods[class_name]
Enter fullscreen mode Exit fullscreen mode

That means deleting or moving a method out of EE::Project fails RuboCop unless you also remove it from legacy_methods.yml—ensuring the baseline list only ever shrinks.

One more rule from software_design.md: do not extract methods into an ActiveSupport::Concern mixin. A concern still injects methods into Project's ancestor chain via inheritance. Instead, move the behavior out via composition into a module or domain class inside the bounded context.

Before: Domain Logic Mixed into EE::Project

In ee/app/models/ee/project.rb, AI Catalog availability lived directly on the ActiveRecord model extension:

# Before (ee/app/models/ee/project.rb)
def ai_catalog_available?
  duo_features_enabled && ::Gitlab::Llm::StageCheck.available?(self, :ai_catalog)
end

def ai_catalog_provisioning_available?
  duo_features_enabled || !project_setting&.duo_features_enabled_locked?
end
Enter fullscreen mode Exit fullscreen mode

Meanwhile, ee/lib/ai/catalog.rb already existed as the bounded context for AI Catalog availability, exposing Ai::Catalog.available?(user) and Ai::Catalog.mcp_servers_available?(user). Splitting the domain rules across EE::Project and Ai::Catalog forced DeclarativePolicy rules, push services, and Sidekiq workers to ask project for AI Catalog state in one place and Ai::Catalog for user state in another.

Design Evolution During Review: Explicit Contracts Over Ambiguous Overloading

Initially, we extracted project availability into Ai::Catalog, and the open question was API ergonomics: should Ai::Catalog.available? inspect the argument type (User vs Project), or should user and project checks have distinct method names?

Reviewer @djadmin and maintainer @eugielimpin both pushed for explicit separate contracts rather than runtime type branching:

  • Rename Ai::Catalog.available?(user) -> Ai::Catalog.available_for_user?(user)
  • Introduce Ai::Catalog.available_for_project?(project)
  • Introduce Ai::Catalog.provisioning_available?(project)

Here is the final bounded-context API in ee/lib/ai/catalog.rb:

module Ai
  module Catalog
    class << self
      def available_for_user?(user)
        return false unless user

        feature_available? &&
          duo_agent_platform_available_for_user?(user) &&
          user.allowed_to_use_through_namespace?(:ai_catalog)
      end

      def available_for_project?(project)
        return false unless project

        project.duo_features_enabled && ::Gitlab::Llm::StageCheck.available?(project, :ai_catalog)
      end

      def provisioning_available?(project)
        return false unless project

        project.duo_features_enabled || !project.project_setting&.duo_features_enabled_locked?
      end

      def mcp_servers_available?(user)
        ::Feature.enabled?(:ai_catalog_mcp_servers, user) &&
          available_for_user?(user) &&
          user_can_access_experimental_and_beta_features?(user)
      end
Enter fullscreen mode Exit fullscreen mode

Why Separate Methods Beat Overloading in Ruby/Rails

  1. Nil safety is unambiguous: Calling available?(nil) raises a question—was a User missing or a Project missing? With available_for_user?(nil) and available_for_project?(nil), both explicitly return false without guessing.
  2. Policy readability: Look at how clean DeclarativePolicy conditions in ee/app/policies/ee/project_policy.rb and ee/app/policies/ee/global_policy.rb become: 
# ee/app/policies/ee/project_policy.rb
condition(:ai_catalog_available, scope: :subject) do
  ::Ai::Catalog.available_for_project?(@subject)
end

condition(:provision_ai_catalog_allowed, scope: :subject) do
  ::Ai::Catalog.provisioning_available?(@subject)
end
Enter fullscreen mode Exit fullscreen mode 
# ee/app/policies/ee/global_policy.rb
condition(:ai_catalog_available, scope: :user) do
  ::Ai::Catalog.available_for_user?(@user)
end
Enter fullscreen mode Exit fullscreen mode
  1. Background worker clarity: In ee/app/workers/ai/catalog/skills/publish_worker.rb and ee/app/services/ee/git/branch_push_service.rb, the domain boundary is immediately visible at the call site: 
return unless ::Ai::Catalog.available_for_project?(project)
Enter fullscreen mode Exit fullscreen mode

We also updated every caller and test stub across 15 files, including:

  • ee/app/policies/ai/catalog/item_policy.rb
  • ee/lib/ai/catalog/flows/inherited_project_authorization.rb
  • ee/spec/support/ai/catalog/test_helpers.rb
  • ee/spec/policies/{global_policy_spec.rb,project_policy_spec.rb}
  • ee/spec/requests/api/graphql/mutations/ai/catalog/item_report_spec.rb
  • rubocop/cop/gitlab/config/omniscient_classes/ee_project/legacy_methods.yml (removed ai_catalog_available? and ai_catalog_provisioning_available?)

The RSpec Gotcha: Why let_it_be(:project) Breaks When Specs Mutate Attributes

While adapting the old ee/spec/models/ee/project_spec.rb examples into ee/spec/lib/ai/catalog_spec.rb, we initially reached for GitLab's performance habit (test-prof's let_it_be) for building the subject fixture:

# Subtle bug when combined with attribute mutation!
describe '.available_for_project?' do
  let_it_be(:project) { build(:project) }

  with_them do
    before do
      allow(::Gitlab::Llm::StageCheck).to receive(:available?)
        .with(project, :ai_catalog).and_return(stage_check_available)
      project.duo_features_enabled = duo_features_enabled
    end
    ...
  end
end
Enter fullscreen mode Exit fullscreen mode

Here is the trap every GitLab Rails engineer hits sooner or later:

  • let_it_be allocates the object once for the entire describe group (outside individual examples).
  • Unlike let_it_be_with_reload on persisted records (which reloads database attributes per example, though still not unpersisted Ruby state or build(...) instances!), an unpersisted build(:project) object created via let_it_be retains mutations across examples.
  • When row 1 mutates project.duo_features_enabled = true, or tests freeze/inspect objects under RSpec::BeforeAll, subsequent examples inherit mutated in-memory state instead of a clean factory instance. Even in .provisioning_available?, using allow(project).to receive(...) on a let_it_be instance couples lifecycle cleanup to before_all vs per-example RSpec mock verification.

Using standard per-example let(:project) { build(:project) } (matching the original ee/spec/models/ee/project_spec.rb) builds an isolated in-memory object in microseconds with zero DB writes and zero shared-state leakage:

describe '.available_for_project?' do
  let(:project) { build(:project) }

  subject(:available?) { described_class.available_for_project?(project) }

  context 'when project is nil' do
    let(:project) { nil }

    it { is_expected.to be(false) }
  end

  where(:stage_check_available, :duo_features_enabled, :expectation) do
    true  | true  | true
    true  | false | false
    false | true  | false
    false | false | false
  end

  with_them do
    before do
      allow(::Gitlab::Llm::StageCheck).to receive(:available?)
        .with(project, :ai_catalog).and_return(stage_check_available)
      project.duo_features_enabled = duo_features_enabled
    end

    it { is_expected.to be expectation }
  end
end
Enter fullscreen mode Exit fullscreen mode

In commit 52b08feaab, we switched both new describe blocks to let(:project) { build(:project) }, verified a 100% green Tier-3 pipeline (345 jobs, 45 downstream predictive bridges), received approval from @eugielimpin, and !269060 merged into master.

Takeaways for GitLab Engineers

  1. Check legacy_methods.yml whenever deleting methods from Project, Group, User, or Organizations::Organization (and their EE:: modules): Gitlab/OmniscientClassMethods enforces monotonicity—removing a method requires deleting its line from legacy_methods.yml in the same commit.
  2. Prefer explicit names over type-switched booleans: available_for_user?(user) + available_for_project?(project) makes callers, policy conditions, and nil semantics self-documenting.
  3. Never wrap build(...) with mutating before hooks in let_it_be: Reserve let_it_be for immutable DB-persisted fixtures (create(...)); use plain let for mutated build(...) stubs.

Series Navigation

Top comments (0)