Review comments updated

Author: thavaahariharangit

npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/version_resolver.rb

@@ -34,6 +34,11 @@ class VersionResolver
           T::Hash[String, T::Array[String]]
         )
 
+        # Maximum number of older versions to check when the latest version
+        # triggers a pnpm trust downgrade. Prevents excessive subprocess spawning
+        # for packages with many published versions.
+        MAX_TRUST_DOWNGRADE_FALLBACK_ATTEMPTS = 5
+
         # Error message returned by `yarn add` (for Yarn classic):
         # " > @reach/router@1.2.1" has incorrect peer dependency "react@15.x || 16.x || 16.4.0-alpha.0911da3"
         # "workspace-aggregator-<random-string> > test > react-dom@15.6.2" has incorrect peer dependency "react@^15.6.2"
@@ -251,6 +256,11 @@ def dependency_updates_from_full_unlock
         sig { returns(T::Boolean) }
         attr_reader :raise_on_ignored
 
+        sig { returns(T::Boolean) }
+        def trust_downgrade_detected?
+          @trust_downgrade_detected
+        end
+
         sig { params(dep: Dependabot::Dependency).returns(PackageLatestVersionFinder) }
         def latest_version_finder(dep)
           @latest_version_finder[dep] ||=
@@ -277,6 +287,7 @@ def find_version_without_trust_downgrade
                                .select { |v| current_version.nil? || v > current_version }
                                .sort
                                .reverse
+                               .first(MAX_TRUST_DOWNGRADE_FALLBACK_ATTEMPTS)
 
           candidate_versions.each do |candidate|
             next if version_has_trust_downgrade?(candidate)
@@ -288,29 +299,37 @@ def find_version_without_trust_downgrade
           end
 
           Dependabot.logger.info(
-            "No version of #{dependency.name} found without pnpm trust downgrade"
+            "No version of #{dependency.name} found without pnpm trust downgrade " \
+            "(checked #{candidate_versions.length} versions)"
           )
           nil
         end
 
         # Checks whether a specific version triggers pnpm trust downgrade by running
         # the peer dependency check and inspecting the trust_downgrade_detected flag.
+        # Saves and restores instance state to avoid polluting the caller's context.
         sig { params(version: Gem::Version).returns(T::Boolean) }
         def version_has_trust_downgrade?(version)
+          saved_trust_downgrade = @trust_downgrade_detected
+          saved_peer_errors = @peer_dependency_errors
+
           @trust_downgrade_detected = false
           @peer_dependency_errors = nil
 
           fetch_peer_dependency_errors(version: version)
 
-          # fetch_peer_dependency_errors sets @trust_downgrade_detected as a side effect
-          # when pnpm returns ERR_PNPM_TRUST_DOWNGRADE
-          result = T.unsafe(@trust_downgrade_detected)
+          result = trust_downgrade_detected?
           if result
             Dependabot.logger.info(
               "pnpm trust downgrade also detected for #{dependency.name}@#{version}, skipping"
             )
           end
           result
+        ensure
+          # T.must needed because Sorbet considers locals potentially nil in ensure blocks;
+          # @peer_dependency_errors doesn't need it since its type is already nilable.
+          @trust_downgrade_detected = T.must(saved_trust_downgrade)
+          @peer_dependency_errors = saved_peer_errors
         end
 
         # rubocop:disable Metrics/PerceivedComplexity

npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb

@@ -1301,6 +1301,67 @@
             expect(result).to eq(Gem::Version.new("1.1.3"))
           end
         end
+
+        context "when fallback limit is exceeded" do
+          before do
+            # Stub the constant to 2 so we only check 2 candidates (1.2.0, 1.1.3)
+            stub_const(
+              "Dependabot::NpmAndYarn::UpdateChecker::VersionResolver::MAX_TRUST_DOWNGRADE_FALLBACK_ATTEMPTS", 2
+            )
+
+            # 1.2.0 and 1.1.3 are checked (within limit) but both fail;
+            # versions below are truncated by the limit.
+            allow(Dependabot::NpmAndYarn::Helpers).to receive(:run_pnpm_command) do |command, **_kwargs|
+              if command.include?("@1.3.0") || command.include?("@1.2.0") || command.include?("@1.1.3")
+                raise Dependabot::SharedHelpers::HelperSubprocessFailed.new(
+                  message: "ERR_PNPM_TRUST_DOWNGRADE  High-risk trust downgrade for " \
+                           "\"left-pad\" (possible package takeover)",
+                  error_context: {}
+                )
+              end
+              "" # Would pass, but not reached due to the limit
+            end
+          end
+
+          it "returns nil after checking only the limited number of versions" do
+            expect(resolver.latest_resolvable_version).to be_nil
+          end
+        end
+
+        context "when the only passing version equals the current version" do
+          let(:dependency) do
+            Dependabot::Dependency.new(
+              name: "left-pad",
+              version: "1.2.0",
+              requirements: [{
+                file: "package.json",
+                requirement: "^1.0.1",
+                groups: ["dependencies"],
+                source: { type: "registry", url: "https://registry.npmjs.org" }
+              }],
+              package_manager: "npm_and_yarn"
+            )
+          end
+
+          before do
+            # All versions above 1.2.0 (only 1.3.0 in this fixture) fail trust downgrade,
+            # and candidates below current version are filtered out.
+            allow(Dependabot::NpmAndYarn::Helpers).to receive(:run_pnpm_command) do |command, **_kwargs|
+              if command.include?("@1.3.0")
+                raise Dependabot::SharedHelpers::HelperSubprocessFailed.new(
+                  message: "ERR_PNPM_TRUST_DOWNGRADE  High-risk trust downgrade for " \
+                           "\"left-pad@1.3.0\" (possible package takeover)",
+                  error_context: {}
+                )
+              end
+              ""
+            end
+          end
+
+          it "returns nil to avoid a no-op update" do
+            expect(resolver.latest_resolvable_version).to be_nil
+          end
+        end
       end
     end
   end