Merge pull request #13899 from dependabot/nishnha/fix-pr-directory-comparison

Nishnha/fix pr directory comparison

Author: jurre

updater/lib/dependabot/updater/group_update_creation.rb

@@ -534,7 +534,34 @@ def cleanup_workspace
 
       sig { params(group: Dependabot::DependencyGroup).returns(T::Boolean) }
       def pr_exists_for_dependency_group?(group)
-        job.existing_group_pull_requests.any? { |pr| pr["dependency-group-name"] == group.name }
+        !find_existing_group_pr(group).nil?
+      end
+
+      sig { params(group: Dependabot::DependencyGroup).returns(T.nilable(T::Hash[String, T.untyped])) }
+      def find_existing_group_pr(group)
+        job.existing_group_pull_requests.find do |pr|
+          next false unless pr["dependency-group-name"] == group.name
+
+          existing_pr_covers_job_directories?(pr)
+        end
+      end
+
+      sig { params(pull_request: T::Hash[String, T.untyped]).returns(T::Boolean) }
+      def existing_pr_covers_job_directories?(pull_request)
+        dependencies = pull_request["dependencies"]
+
+        # Old PRs without directory info — treat as match (backward compat).
+        # Only enforce directory matching when ALL dependencies include a directory,
+        # consistent with DependencyChange#matches_existing_pr? and PullRequest#using_directory?.
+        return true if dependencies.nil? || !dependencies.all? { |dep| dep["directory"] }
+
+        pr_directories = dependencies.filter_map { |dep| dep["directory"] }
+        job_directories = job.source.directories || [job.source.directory || "/"]
+        normalized_job_dirs = job_directories.map { |d| Pathname.new(d).cleanpath.to_s }.uniq
+        normalized_pr_dirs = pr_directories.map { |d| Pathname.new(d).cleanpath.to_s }.uniq
+
+        # Match only when the PR directories exactly match the job directories
+        normalized_job_dirs.sort == normalized_pr_dirs.sort
       end
 
       sig do

updater/lib/dependabot/updater/operations/group_update_all_versions.rb

@@ -85,9 +85,9 @@ def run_grouped_dependency_updates
           # Preprocess to discover existing group PRs and add their dependencies to the handled list before processing
           # the rest of the groups. This prevents multiple PRs from being created for the same dependency.
           groups_without_pr = dependency_snapshot.groups.filter_map do |group|
-            if pr_exists_for_dependency_group?(group)
-              existing_pr = job.existing_group_pull_requests.find { |pr| pr["dependency-group-name"] == group.name }
-              pr_number = existing_pr["pr_number"] if existing_pr
+            existing_pr = find_existing_group_pr(group)
+            if existing_pr
+              pr_number = existing_pr["pr_number"]
 
               Dependabot.logger.info(
                 "Detected existing pull request ##{pr_number} for the dependency group '#{group.name}'."

updater/spec/dependabot/pull_request_spec.rb

@@ -278,5 +278,93 @@
 
       expect(pr1).not_to eq(pr2)
     end
+
+    it "is true when one has directory and the other doesn't" do
+      pr1 = described_class.new(
+        [
+          Dependabot::PullRequest::Dependency.new(
+            name: "foo",
+            version: "1.0.0",
+            directory: "/foo"
+          )
+        ],
+        pr_number: 123
+      )
+      pr2 = described_class.new(
+        [
+          Dependabot::PullRequest::Dependency.new(
+            name: "foo",
+            version: "1.0.0"
+          )
+        ],
+        pr_number: 456
+      )
+
+      expect(pr1).to eq(pr2)
+    end
+
+    it "is false when directories are different" do
+      existing_pr = described_class.new(
+        [
+          Dependabot::PullRequest::Dependency.new(
+            name: "rollup",
+            version: "2.79.2",
+            directory: "/packages/corelib"
+          )
+        ],
+        pr_number: 123
+      )
+      new_pr = described_class.new(
+        [
+          Dependabot::PullRequest::Dependency.new(
+            name: "rollup",
+            version: "2.79.2",
+            directory: "/"
+          )
+        ]
+      )
+
+      expect(existing_pr).not_to eq(new_pr)
+    end
+
+    it "is false when comparing PRs from job definition with different directories" do
+      existing_prs = described_class.create_from_job_definition(
+        existing_pull_requests: [
+          [{ "dependency-name" => "rollup", "dependency-version" => "2.79.2", "directory" => "/packages/corelib" }]
+        ]
+      )
+
+      updated_dependency = instance_double(
+        Dependabot::Dependency,
+        name: "rollup",
+        version: "2.79.2",
+        removed?: false,
+        directory: "/."
+      )
+      new_pr = described_class.create_from_updated_dependencies([updated_dependency])
+
+      expect(existing_prs.find { |pr| pr == new_pr }).to be_nil
+    end
+
+    it "is true when existing PR has no directory but new PR does" do
+      existing_prs = described_class.create_from_job_definition(
+        existing_pull_requests: [
+          [{ "dependency-name" => "rollup", "dependency-version" => "2.79.2" }]
+        ]
+      )
+
+      updated_dependency = instance_double(
+        Dependabot::Dependency,
+        name: "rollup",
+        version: "2.79.2",
+        removed?: false,
+        directory: "/."
+      )
+      new_pr = described_class.create_from_updated_dependencies([updated_dependency])
+
+      expect(existing_prs.first.using_directory?).to be false
+      expect(new_pr.using_directory?).to be true
+      expect(existing_prs.find { |pr| pr == new_pr }).not_to be_nil
+    end
   end
 end

updater/spec/dependabot/updater/operations/group_update_all_versions_spec.rb

@@ -284,6 +284,87 @@
         end
       end
 
+      context "when PR exists for same group but different directory" do
+        before do
+          allow(job).to receive_messages(
+            existing_group_pull_requests: [
+              {
+                "dependency-group-name" => "dummy-group",
+                "pr_number" => 123,
+                "dependencies" => [
+                  {
+                    "dependency-name" => "rollup",
+                    "dependency-version" => "2.79.2",
+                    "directory" => "/packages/corelib"
+                  }
+                ]
+              }
+            ],
+            source: mock_source
+          )
+          allow(mock_source).to receive(:directory).and_return("/")
+        end
+
+        it "creates a new PR for the different directory" do
+          allow(mock_create_group_update).to receive(:perform).and_return(mock_dependency_change)
+          expect(mock_create_group_update).to receive(:perform)
+          expect(dependency_snapshot).not_to receive(:mark_group_handled).with(dependency_group)
+          perform
+        end
+      end
+
+      context "when PR exists for same group and same directory" do
+        before do
+          allow(job).to receive_messages(
+            existing_group_pull_requests: [
+              {
+                "dependency-group-name" => "dummy-group",
+                "pr_number" => 123,
+                "dependencies" => [
+                  {
+                    "dependency-name" => "rollup",
+                    "dependency-version" => "2.79.2",
+                    "directory" => "/"
+                  }
+                ]
+              }
+            ],
+            source: mock_source
+          )
+          allow(mock_source).to receive(:directory).and_return("/")
+        end
+
+        it "skips creating a new PR" do
+          expect(mock_create_group_update).not_to receive(:perform)
+          expect(dependency_snapshot).to receive(:mark_group_handled).with(dependency_group)
+          perform
+        end
+      end
+
+      context "when existing PR has no directory info" do
+        before do
+          allow(job).to receive_messages(
+            existing_group_pull_requests: [
+              {
+                "dependency-group-name" => "dummy-group",
+                "pr_number" => 123,
+                "dependencies" => [
+                  { "dependency-name" => "rollup", "dependency-version" => "2.79.2" }
+                ]
+              }
+            ],
+            source: mock_source
+          )
+          allow(mock_source).to receive(:directory).and_return("/")
+        end
+
+        it "treats the existing PR as a match" do
+          expect(mock_create_group_update).not_to receive(:perform)
+          expect(dependency_snapshot).to receive(:mark_group_handled).with(dependency_group)
+          perform
+        end
+      end
+
       context "when group update fails" do
         before do
           allow(mock_create_group_update).to receive(:perform).and_return(nil)