Replace transparent find_by resolution with ArgumentError guard

find_by(id:) and find_by!(id:) now raise ArgumentError when passed
a semantic identifier string, directing developers to use find() or
the new find_by_display_id() method instead. This avoids silently
altering ActiveRecord's find_by semantics and ensures misuse is
caught in development even when semantic mode is not enabled locally.

Renames find_by_id_or_identifier to find_by_display_id (public API)
and migrates all app callers that receive user-facing strings to use
the new method.

.rubocop.yml

@@ -170,8 +170,8 @@ Rails/ContentTag:
 Rails/DynamicFindBy:
   Enabled: true
   AllowedMethods:
-    - find_by_id_or_identifier
-    - find_by_id_or_identifier!
+    - find_by_display_id
+    - find_by_display_id!
     - find_by_semantic_identifier
   Exclude:
     - "spec/features/**/*.rb"

app/components/work_packages/full_view/show_component.rb

@@ -13,7 +13,7 @@ module WorkPackages
 
         @id = id
         @tab = tab
-        @work_package = WorkPackage.visible.find_by(id:)
+        @work_package = WorkPackage.visible.find_by_display_id(id)
       end
 
       def wrapper_uniq_by

app/components/work_packages/hover_card_component.rb

@@ -35,7 +35,7 @@ class WorkPackages::HoverCardComponent < ApplicationComponent
     super
 
     @id = id
-    @work_package = WorkPackage.visible.find_by(id:)
+    @work_package = WorkPackage.visible.find_by_display_id(id)
     @assignee = @work_package.present? ? @work_package.assigned_to : nil
   end
 

app/components/work_packages/split_view_component.rb

@@ -39,7 +39,7 @@ class WorkPackages::SplitViewComponent < ApplicationComponent
 
     @id = id
     @tab = tab
-    @work_package = WorkPackage.visible.find_by(id:)
+    @work_package = WorkPackage.visible.find_by_display_id(id)
     @base_route = base_route
   end
 

app/controllers/work_packages_controller.rb

@@ -243,7 +243,7 @@ class WorkPackagesController < ApplicationController
   def work_package
     return @work_package if defined?(@work_package)
 
-    @work_package = WorkPackage.visible(current_user).find_by(id: params[:id])
+    @work_package = WorkPackage.visible(current_user).find_by_display_id(params[:id])
   end
 
   def journals

app/models/queries/work_packages/filter/relatable_filter.rb

@@ -49,7 +49,7 @@ class Queries::WorkPackages::Filter::RelatableFilter < Queries::WorkPackages::Fi
   end
 
   def apply_to(query_scope)
-    query_scope.relatable(WorkPackage.visible.find_by(id: values.first), scope_operator)
+    query_scope.relatable(WorkPackage.visible.find_by_display_id(values.first), scope_operator)
   end
 
   private

app/models/work_package/exports/macros/attributes.rb

@@ -120,7 +120,7 @@ module WorkPackage::Exports
         return resolve_label_work_package(attribute) if type == "label"
         return msg_macro_error(I18n.t("export.macro.model_not_found", model: type)) unless type == "value"
 
-        work_package = WorkPackage.visible(user).find_by(id:)
+        work_package = WorkPackage.visible(user).find_by_display_id(id)
         if work_package.nil?
           return msg_macro_error(I18n.t("export.macro.resource_not_found", resource: "#{WorkPackage.name} #{id}"))
         end

app/models/work_package/semantic_identifier/finder_methods.rb

@@ -28,56 +28,43 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-# Extends ActiveRecord finder methods (find, find_by, find_by!, exists?) to
-# transparently resolve semantic work package identifiers (e.g. "PROJ-42")
-# in addition to numeric IDs.
+# Extends ActiveRecord finder methods to support semantic work package
+# identifiers (e.g. "PROJ-42") in addition to numeric IDs.
+#
+# - find("PROJ-42") resolves transparently
+# - find_by(id:)/find_by!(id:) raise ArgumentError for semantic strings
+# - find_by_display_id("PROJ-42") is the explicit nil-on-miss resolver
+# - exists?("PROJ-42") resolves transparently
 #
 # Included into WorkPackage class methods and extended into every
 # ActiveRecord::Relation via WorkPackage::SemanticIdentifier.
-#
-# Examples:
-#   WorkPackage.find("PROJ-42")
-#   WorkPackage.visible(user).find_by(id: "PROJ-42")
-#   WorkPackage.exists?("PROJ-42")
 module WorkPackage::SemanticIdentifier::FinderMethods
   def find(*args)
     ids = args.length == 1 && args.first.is_a?(Array) ? args.first : args
 
     if ids.length == 1 && semantic_id?(ids.first)
-      return find_by_id_or_identifier!(ids.first)
+      return find_by_display_id!(ids.first)
     end
 
     if ids.any? { |id| semantic_id?(id) }
       raise ArgumentError,
             "Semantic identifiers in multi-argument find is not yet supported. " \
-            "Resolve each identifier individually via find_by(id:) instead."
+            "Resolve each identifier individually via find_by_display_id instead."
     end
 
     super
   end
 
-  # Override find_by to transparently resolve semantic identifiers when called
-  # with `id:` as the sole keyword (e.g. `find_by(id: "PROJ-42")`).
-  # All other find_by calls pass through to ActiveRecord unchanged.
-  #
-  # AR's find_by signature is find_by(arg, *args) — it doesn't use keyword splat,
-  # so hash kwargs arrive as the positional `arg`. We match on that.
+  # Guard find_by against semantic identifiers passed via `id:` or `identifier:`.
+  # Developers should use find("PROJ-42") or find_by_display_id("PROJ-42") instead.
   def find_by(*args)
-    if semantic_id_hash_lookup?(args)
-      find_by_id_or_identifier(args.first[:id])
-    else
-      super
-    end
+    reject_semantic_id_in_find_by!(args)
+    super
   end
 
-  # Mirror of find_by — Rails implements find_by! independently (not via find_by),
-  # so we must override both to keep the pair consistent.
   def find_by!(*args)
-    if semantic_id_hash_lookup?(args)
-      find_by_id_or_identifier!(args.first[:id])
-    else
-      super
-    end
+    reject_semantic_id_in_find_by!(args)
+    super
   end
 
   def exists?(conditions = :none)
@@ -86,36 +73,39 @@ module WorkPackage::SemanticIdentifier::FinderMethods
     exists_by_semantic_identifier?(conditions)
   end
 
-  private
-
-  # Returns true when args represent a single `id:` keyword lookup
-  # with a semantic identifier value (e.g. `find_by(id: "PROJ-42")`).
-  def semantic_id_hash_lookup?(args)
-    args.length == 1 &&
-      args.first.is_a?(Hash) &&
-      args.first.keys == [:id] &&
-      semantic_id?(args.first[:id])
-  end
-
-  # Resolves any identifier form to a WorkPackage.
+  # Resolves any display-facing identifier to a WorkPackage.
   #   - Numeric string ("12345")    → find by primary key
-  #   - Semantic string ("PROJ-42") → lookup via work_packages table and alias table
+  #   - Semantic string ("PROJ-42") → lookup via identifier column and alias table
   #
   # Returns nil on miss.
-  def find_by_id_or_identifier(identifier)
+  def find_by_display_id(identifier)
     return find_by(id: identifier) unless semantic_id?(identifier)
 
     find_by_semantic_identifier(identifier)
   end
 
-  # Same as find_by_id_or_identifier but raises ActiveRecord::RecordNotFound on miss.
-  def find_by_id_or_identifier!(identifier)
-    find_by_id_or_identifier(identifier) ||
+  # Same as find_by_display_id but raises ActiveRecord::RecordNotFound on miss.
+  def find_by_display_id!(identifier)
+    find_by_display_id(identifier) ||
       raise(ActiveRecord::RecordNotFound.new(
               "Couldn't find WorkPackage with identifier=#{identifier}", "WorkPackage", "identifier", identifier
             ))
   end
 
+  private
+
+  def reject_semantic_id_in_find_by!(args)
+    return unless args.length == 1 && args.first.is_a?(Hash)
+
+    hash = args.first
+    key, value = hash.assoc(:id) || hash.assoc(:identifier)
+    return unless key && semantic_id?(value)
+
+    raise ArgumentError,
+          "Semantic identifier #{value.inspect} cannot be passed to find_by(#{key}:). " \
+          "Use find(#{value.inspect}) or find_by_display_id(#{value.inspect}) instead."
+  end
+
   # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42").
   # Non-string values (Integer, Hash, nil, Array) and numeric strings ("123", " 456 ")
   # return false — these fall through to standard ActiveRecord lookup.

app/services/mcp_resources/work_package.rb

@@ -37,7 +37,7 @@ module McpResources
     default_description "Access work packages of this OpenProject instance."
 
     def read(id:)
-      work_package = ::WorkPackage.visible.find_by(id:)
+      work_package = ::WorkPackage.visible.find_by_display_id(id)
       return nil if work_package.nil?
 
       API::V3::WorkPackages::WorkPackageRepresenter.create(work_package, current_user:, embed_links: true)

spec/models/work_package/semantic_identifier_spec.rb

@@ -170,17 +170,16 @@ RSpec.describe WorkPackage::SemanticIdentifier do
 
   describe "WorkPackage.find_by" do
     context "with id: keyword and semantic identifier" do
-      it "resolves via the semantic identifier" do
-        expect(WorkPackage.find_by(id: "MYPROJ-1")).to eq(work_package)
+      it "raises ArgumentError pointing to find_by_display_id" do
+        expect { WorkPackage.find_by(id: "MYPROJ-1") }
+          .to raise_error(ArgumentError, /find_by_display_id/)
       end
+    end
 
-      it "returns nil for unknown semantic id" do
-        expect(WorkPackage.find_by(id: "MYPROJ-999")).to be_nil
-      end
-
-      it "resolves via historic alias" do
-        WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:)
-        expect(WorkPackage.find_by(id: "OLDPROJ-1")).to eq(work_package)
+    context "with identifier: keyword and semantic identifier" do
+      it "raises ArgumentError pointing to find_by_display_id" do
+        expect { WorkPackage.find_by(identifier: "MYPROJ-1") }
+          .to raise_error(ArgumentError, /find_by_display_id/)
       end
     end
 
@@ -198,37 +197,14 @@ RSpec.describe WorkPackage::SemanticIdentifier do
 
     context "with multiple keywords including id:" do
       it "passes through to standard AR find_by" do
-        expect(WorkPackage.find_by(id: work_package.id, project: project)).to eq(work_package)
+        expect(WorkPackage.find_by(id: work_package.id, project:)).to eq(work_package)
       end
     end
 
     context "with an unparseable semantic string" do
-      it "returns nil" do
-        expect(WorkPackage.find_by(id: "not-an-identifier!")).to be_nil
-      end
-    end
-
-    context "when identifier differs but an alias row exists" do
-      it "resolves via the alias table" do
-        work_package.update_columns(identifier: "OTHER-99")
-        expect(WorkPackage.find_by(id: "MYPROJ-1")).to eq(work_package)
-      end
-    end
-
-    context "with visibility scoping" do
-      let(:member_user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) }
-      let(:non_member_user) { create(:user) }
-
-      it "respects the scope for semantic ids" do
-        expect(WorkPackage.visible(member_user).find_by(id: "MYPROJ-1")).to eq(work_package)
-      end
-
-      it "returns nil when the user cannot see it" do
-        expect(WorkPackage.visible(non_member_user).find_by(id: "MYPROJ-1")).to be_nil
-      end
-
-      it "also scopes numeric lookup" do
-        expect(WorkPackage.visible(non_member_user).find_by(id: work_package.id.to_s)).to be_nil
+      it "raises ArgumentError" do
+        expect { WorkPackage.find_by(id: "not-an-identifier!") }
+          .to raise_error(ArgumentError, /find_by_display_id/)
       end
     end
   end
@@ -236,12 +212,9 @@ RSpec.describe WorkPackage::SemanticIdentifier do
   # rubocop:disable Rails/FindById -- testing find_by! override specifically, not suggesting find()
   describe "WorkPackage.find_by!" do
     context "with id: keyword and semantic identifier" do
-      it "resolves via the semantic identifier" do
-        expect(WorkPackage.find_by!(id: "MYPROJ-1")).to eq(work_package)
-      end
-
-      it "raises RecordNotFound for unknown semantic id" do
-        expect { WorkPackage.find_by!(id: "MYPROJ-999") }.to raise_error(ActiveRecord::RecordNotFound)
+      it "raises ArgumentError pointing to find_by_display_id" do
+        expect { WorkPackage.find_by!(id: "MYPROJ-1") }
+          .to raise_error(ArgumentError, /find_by_display_id/)
       end
     end
 
@@ -253,6 +226,73 @@ RSpec.describe WorkPackage::SemanticIdentifier do
   end
   # rubocop:enable Rails/FindById
 
+  describe "WorkPackage.find_by_display_id" do
+    context "with a semantic identifier" do
+      it "resolves via the semantic identifier" do
+        expect(WorkPackage.find_by_display_id("MYPROJ-1")).to eq(work_package)
+      end
+
+      it "returns nil for unknown semantic id" do
+        expect(WorkPackage.find_by_display_id("MYPROJ-999")).to be_nil
+      end
+
+      it "resolves via historic alias" do
+        WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:)
+        expect(WorkPackage.find_by_display_id("OLDPROJ-1")).to eq(work_package)
+      end
+
+      it "resolves when identifier column differs but alias row exists" do
+        work_package.update_columns(identifier: "OTHER-99")
+        expect(WorkPackage.find_by_display_id("MYPROJ-1")).to eq(work_package)
+      end
+    end
+
+    context "with a numeric string" do
+      it "falls through to standard AR find_by" do
+        expect(WorkPackage.find_by_display_id(work_package.id.to_s)).to eq(work_package)
+      end
+
+      it "returns nil for unknown numeric id" do
+        expect(WorkPackage.find_by_display_id("9999999")).to be_nil
+      end
+    end
+
+    context "with visibility scoping" do
+      let(:member_user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) }
+      let(:non_member_user) { create(:user) }
+
+      it "respects the scope for semantic ids" do
+        expect(WorkPackage.visible(member_user).find_by_display_id("MYPROJ-1")).to eq(work_package)
+      end
+
+      it "returns nil when the user cannot see it" do
+        expect(WorkPackage.visible(non_member_user).find_by_display_id("MYPROJ-1")).to be_nil
+      end
+
+      it "also scopes numeric lookup" do
+        expect(WorkPackage.visible(non_member_user).find_by_display_id(work_package.id.to_s)).to be_nil
+      end
+    end
+  end
+
+  describe "WorkPackage.find_by_display_id!" do
+    context "with a semantic identifier" do
+      it "resolves via the semantic identifier" do
+        expect(WorkPackage.find_by_display_id!("MYPROJ-1")).to eq(work_package)
+      end
+
+      it "raises RecordNotFound for unknown semantic id" do
+        expect { WorkPackage.find_by_display_id!("MYPROJ-999") }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
+    context "with a numeric string" do
+      it "falls through to standard AR find" do
+        expect(WorkPackage.find_by_display_id!(work_package.id.to_s)).to eq(work_package)
+      end
+    end
+  end
+
   describe "#display_id" do
     context "when semantic mode is active",
             with_flag: { semantic_work_package_ids: true },

spec/services/work_packages/semantic_ids/integration_spec.rb

@@ -97,12 +97,12 @@ RSpec.describe "SemanticIds registry integration", type: :model do
 
     it "old identifier still resolves to the WP" do
       WorkPackages::UpdateService.new(user:, model: work_package).call(project: target_project)
-      expect(WorkPackage.find_by(id: "PROJ-5")).to eq(work_package)
+      expect(WorkPackage.find_by_display_id("PROJ-5")).to eq(work_package)
     end
 
     it "new identifier also resolves to the WP" do
       WorkPackages::UpdateService.new(user:, model: work_package).call(project: target_project)
-      expect(WorkPackage.find_by(id: work_package.reload.identifier)).to eq(work_package)
+      expect(WorkPackage.find_by_display_id(work_package.reload.identifier)).to eq(work_package)
     end
   end
 
@@ -130,15 +130,15 @@ RSpec.describe "SemanticIds registry integration", type: :model do
     it "old identifiers still resolve to the correct WPs" do
       Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED")
 
-      expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1)
-      expect(WorkPackage.find_by(id: "PROJ-2")).to eq(wp2)
+      expect(WorkPackage.find_by_display_id("PROJ-1")).to eq(wp1)
+      expect(WorkPackage.find_by_display_id("PROJ-2")).to eq(wp2)
     end
 
     it "new identifiers resolve to the correct WPs" do
       Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED")
 
-      expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1)
-      expect(WorkPackage.find_by(id: "RENAMED-2")).to eq(wp2)
+      expect(WorkPackage.find_by_display_id("RENAMED-1")).to eq(wp1)
+      expect(WorkPackage.find_by_display_id("RENAMED-2")).to eq(wp2)
     end
 
     it "old prefix resolves for WPs created after the rename" do
@@ -147,8 +147,8 @@ RSpec.describe "SemanticIds registry integration", type: :model do
       Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED")
       wp3 = create(:work_package, project: project.reload)
 
-      expect(WorkPackage.find_by(id: "RENAMED-3")).to eq(wp3)
-      expect(WorkPackage.find_by(id: "PROJ-3")).to eq(wp3)
+      expect(WorkPackage.find_by_display_id("RENAMED-3")).to eq(wp3)
+      expect(WorkPackage.find_by_display_id("PROJ-3")).to eq(wp3)
     end
   end
 
@@ -161,7 +161,7 @@ RSpec.describe "SemanticIds registry integration", type: :model do
       # PROJ is then renamed to RENAMED (bulk-inserts RENAMED-1 from the retired PROJ-1 row)
       Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED")
 
-      expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1)
+      expect(WorkPackage.find_by_display_id("RENAMED-1")).to eq(wp1)
     end
 
     it "rename then move: both old identifiers resolve after the WP moves" do
@@ -170,8 +170,8 @@ RSpec.describe "SemanticIds registry integration", type: :model do
       # WP moves to DEST (appends DEST-1 registry row, updates identifier)
       WorkPackages::UpdateService.new(user:, model: wp1.reload).call(project: target_project)
 
-      expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1)
-      expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1)
+      expect(WorkPackage.find_by_display_id("PROJ-1")).to eq(wp1)
+      expect(WorkPackage.find_by_display_id("RENAMED-1")).to eq(wp1)
     end
 
     it "rename then new WP then move: pre-rename identifier resolves via alias table" do
@@ -183,7 +183,7 @@ RSpec.describe "SemanticIds registry integration", type: :model do
       # wp2 moves to DEST — old identifier RENAMED-2 kept as alias, gets DEST-1
       WorkPackages::UpdateService.new(user:, model: wp2).call(project: target_project)
 
-      expect(WorkPackage.find_by(id: "PROJ-2")).to eq(wp2)
+      expect(WorkPackage.find_by_display_id("PROJ-2")).to eq(wp2)
     end
   end
 
@@ -202,9 +202,9 @@ RSpec.describe "SemanticIds registry integration", type: :model do
       WorkPackages::UpdateService.new(user:, model: wp1.reload).call(project: project_c)
       projc_identifier = wp1.reload.identifier
 
-      expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1)
-      expect(WorkPackage.find_by(id: dest_identifier)).to eq(wp1)
-      expect(WorkPackage.find_by(id: projc_identifier)).to eq(wp1)
+      expect(WorkPackage.find_by_display_id("PROJ-1")).to eq(wp1)
+      expect(WorkPackage.find_by_display_id(dest_identifier)).to eq(wp1)
+      expect(WorkPackage.find_by_display_id(projc_identifier)).to eq(wp1)
     end
   end
 end