Do not close a PR when it supersedes other groups

[judocode]

What are you trying to accomplish?

Prevent this issue, which, in summary:

• When a dependency is in 1 PR, but according to dependabot, it could be in multiple PRs (not in practice because 1 group specifies major and 1 is minor), it incorrectly closes the PR and that dependency won't return to any PR without manual intervention

Anything you want to highlight for special attention from reviewers?

I added an optional param to mark_group_handled called excluding_dependencies, which should work as advertised, and will avoid the exclusion if the dependency is in an existing PR. Also, this should maintain current behavior of closing a PR when a dep exists in more than 1 PR.

I only used this optional param in RefreshGroupUpdatePullRequest, and there are 2 other uses in GroupUpdateAllVersions. We may consider adding excluding_dependencies there as well, but I am not familiar with that path.

How will you know you've accomplished your goal?

I have added a new test along with fixtures that demonstrates this new functionality works. Also, previous functionality and tests are maintained, which include an existing test that will close a PR if it sees a dependency exists in 2 PRs.

Checklist

• I have run the complete test suite to ensure all tests and linters pass.
• I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
• I have written clear and descriptive commit messages.
• I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
• I have ensured that the code is well-documented and easy to understand.

[kbukum1]

This method can be simplified to avoid redundant variables (set and names) and directly update the handled dependencies. Here's a suggestion:

def add_handled_dependencies(dependency_names)
  assert_current_directory_set!
  @handled_dependencies[@current_directory] ||= Set.new
  names = Array(dependency_names)
  Dependabot.logger.info("Adding dependencies as handled: (#{names.join(', ')}).")
  @handled_dependencies[@current_directory] += names
end

[judocode]

Ok done

[judocode]

I also just found a solution to the original problem that I've committed in addition to the logging

[judocode]

Not sure why 2.0.0 did not exist here, but did elsewhere

[abdulapopoola]

Thank you so much @judocode !

[judocode]

@abdulapopoola @kbukum1 is anything else needed here?

[kbukum1]

@judocode ,
This exclusion logic applies globally without checking the directory or update version. This may prevent necessary updates in other directories or skip higher-version updates. See full discussion in PR comments: #11382 (review)

[kbukum1]

@judocode ,

🔹 Concern 1: Cross-directory Exclusion

The exclusion process is currently checking dependencies globally without considering which directory the PR was created for. However, dependencies in different directories may still need updates and should not be excluded incorrectly.

⚠️ Potential Issue

• A dependency in one directory may have an existing PR, but the same dependency in another directory could still require an update.
• Right now, dependencies are excluded without verifying if the PR exists for the same directory, which may prevent necessary updates.

🛠 Suggested Fix

✅ Directory-Specific Exclusion

• Identify PRs per directory and collect exclude dependencies only for that specific directory rather than applying exclusions globally.

---

🔹 Concern 2: Version-based Exclusion

When excluding dependencies, the logic does not check whether the proposed update is for the same version or a higher version.

⚠️ Potential Issue

• If the new update proposes a higher version, we should not exclude it, as it still requires an update.
• Right now, exclusions happen without verifying dependency version in the created PR and proposed new one

🛠 Suggested Fix

✅ Directory-Specific Exclusion

• We somehow needs to consider not only dependencies but their versions as well between existing PR and new one.

Would love to hear your thoughts on these! 🚀

CC: @abdulapopoola

[judocode]

@kbukum1 To the 2nd point: You cannot know the version at this point. You can only know whether a dependency should be in a group, then the version check process will either include/exclude it based on the version. Which in this case, if you have overlapping groups, this update at least makes it possible and will be predictable, which is better than no update because of flawed logic closing PRs.

[kbukum1]

@kbukum1 To the 2nd point: You cannot know the version at this point. You can only know whether a dependency should be in a group, then the version check process will either include/exclude it based on the version. Which in this case, if you have overlapping groups, this update at least makes it possible and will be predictable, which is better than no update because of flawed logic closing PRs.

Thanks for pointing that out. I will re-review it soon.

[kbukum1]

Revoking approval due to a concern. Re-reviewing.

[kbukum1]

LGTM. Thanks for the contribution! 🎉

✅ Review Summary

• Concern 1 (Cross-directory Exclusion): Valid issue, and it's correctly addressed.

  • The fix ensures that dependencies are marked as handled only within the correct directory, preventing unintended exclusions across different directories.

• Concern 2 (Version-based Exclusion): Not an issue.

  • Dependencies should always be included in the refresh process regardless of version, as the update process itself determines whether a higher version is available and applies the update accordingly.

[kbukum1]

@judocode,

Just one last thing—since this is a core change, I’m adding a feature flag on our API so we can manage the rollout. Can you apply the feature flag to your changes?

Feature Flag Name:

allow_refresh_for_existing_pr_dependencies

How to Use the Feature Flag

Feature Flag Check Condition:

You can check whether the feature flag is enabled using:

# enable can be true or false.
enable = Dependabot::Experiments.enabled?(:allow_refresh_for_existing_pr_dependencies)

For Specs:

When writing or updating tests, you can stub the feature flag as follows:

RSpec.describe YourSpecs do
  ...
  before do
    ...
    allow(Dependabot::Experiments).to receive(:enabled?)
      .with(:allow_refresh_for_existing_pr_dependencies)
      .and_return(allow_refresh_for_existing_pr_dependencies)
  end
  ...
end

Note: Other specs may also require this feature flag if it hits the place where you used the feature flag.

Let me know if you have any questions! 🚀

[kbukum1]

@judocode ,

I did update feature flag checks to be on safe side. I will deploy it today after the pipeline succeed. But if you see any issue please let me know.

[kbukum1]

The fix is moved behind a feature flag for a gradual rollout. Since this change affects the core, we’ll monitor it in production and progressively roll it out to 100%.

[kbukum1]

@judocode
The feature flag is rollout 100%. You can test it now on your repository.

CC: @abdulapopoola