Avoid corrupt trees when submodules are present and a specially named subdirectory is a sibling

[newren]

What are you trying to accomplish?

Trying to fix #14346.

When a project has a submodule, updates to any manifest files within that submodule need to be made within the submodule, not as direct file entries within the outer project.

Sadly, the attempts to update the directory entries directly in the outer project were also resulting in corrupt trees (with duplicate entries), which fail fsck checks and trips assertion failures in git merge logic.

Anything you want to highlight for special attention from reviewers?

I am not familiar with dependabot-core code, and I don't actually know much Ruby. I used GitHub Copilot to come up with these patches and help me trace the problem from assertion failures in git-merge-tree, to dependabot, to this repo, to these fixes, to the GitHub API, to the backend code that API calls, to where that backend fails to reject these bad requests from dependabot, and have patches for multiple projects from this. Copilot didn't quite get the patches right for the other projects, so they may not be perfect here either, but the patches were at least helpful at highlighting the problem area for the other projects, so I hope they are similarly helpful here. The individual commit messages should at least help describe the background for each change.

How will you know you've accomplished your goal?

My main goal is fixing the assertion failures in git merging, and making the backend reject the mal-formed tree changes that dependabot is requesting, but those are being tracked in other projects. I'm sending these in so you at least have a heads up that some of your requests will start failing, and perhaps give you a heads up on the patches needed to fix them.

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.

[Copilot]

Pull request overview

Fixes GitHub tree corruption scenarios (duplicate/conflicting tree entries) when Dependabot attempts to write updates beneath paths that are submodules (or symlinks), aligning with the issue report in #14346.

Changes:

• Add filtering in GitHub PR creation/updating to exclude tree entries that would conflict with submodule/symlink entries in the base tree.
• Enhance cloned repository directory listing to classify submodule paths as "submodule" and ensure submodule metadata is populated when reusing an existing clone.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
common/lib/dependabot/pull_request_updater/github.rb	Filters conflicting create_tree entries against base-tree submodules/symlinks before updating PR branches.
common/lib/dependabot/pull_request_creator/github.rb	Filters conflicting create_tree entries against base-tree submodules/symlinks before creating PR commits.
common/lib/dependabot/file_fetchers/base.rb	Detects submodule paths in cloned repos and marks directory entries as "submodule" to avoid treating them as normal directories.

Comments suppressed due to low confidence (6)

common/lib/dependabot/pull_request_creator/github.rb:369

• This logic silently drops updates under a submodule/symlink path and proceeds to create a commit anyway. That can result in a PR/branch update that claims success but is missing intended file changes (or produces a no-op commit with the same tree). Instead of only logging, consider failing fast with a dedicated error when any entries are excluded, so Dependabot surfaces a clear failure rather than generating an incomplete update.

        file_trees.reject do |entry|
          path = T.must(entry[:path])
          first_component = T.must(path.split("/").first)
          if path.include?("/") && special_paths.include?(first_component)
            Dependabot.logger.warn(
              "Excluding #{path} from create_tree — " \
              "conflicts with submodule/symlink '#{first_component}' in base tree"
            )
            true

common/lib/dependabot/pull_request_creator/github.rb:356

• This change introduces new GitHub API calls (git_commit + tree) and new filtering behavior, but the existing PR creator specs stub only POST /git/trees and will start failing due to unexpected requests. Please update/add tests to stub the additional endpoints and to assert the conflict-filtering behavior (including the warning/error path).

      def filter_conflicting_entries(file_trees)
        commit = T.unsafe(github_client_for_source).git_commit(source.repo, base_commit)
        base_tree = T.unsafe(github_client_for_source).tree(source.repo, commit.tree.sha)
        special_paths = T.let(Set.new, T::Set[String])
        base_tree.tree.each do |entry|
          # Submodules have type "commit"; symlinks have mode "120000"
          special_paths.add(entry.path) if entry.type == "commit" || entry.mode == "120000"

common/lib/dependabot/pull_request_creator/github.rb:353

• filter_conflicting_entries is duplicated (nearly identically) in both PullRequestCreator::Github and PullRequestUpdater::Github. To avoid the two implementations drifting (especially around edge cases like nested paths and error handling), consider extracting this into a shared helper/module used by both classes.

      sig do
        params(file_trees: T::Array[T::Hash[Symbol, T.untyped]])
          .returns(T::Array[T::Hash[Symbol, T.untyped]])
      end
      def filter_conflicting_entries(file_trees)
        commit = T.unsafe(github_client_for_source).git_commit(source.repo, base_commit)
        base_tree = T.unsafe(github_client_for_source).tree(source.repo, commit.tree.sha)
        special_paths = T.let(Set.new, T::Set[String])

common/lib/dependabot/pull_request_updater/github.rb:236

• New filtering introduces extra GitHub API calls and behavior changes, but updater specs currently stub only a subset of requests. Please update/add tests to stub git_commit/tree calls for base_commit and to validate the filtering behavior so regressions don’t slip in.

      sig do
        params(file_trees: T::Array[T::Hash[Symbol, T.untyped]])
          .returns(T::Array[T::Hash[Symbol, T.untyped]])
      end
      def filter_conflicting_entries(file_trees)
        commit = T.unsafe(github_client_for_source).git_commit(source.repo, base_commit)
        base_tree = T.unsafe(github_client_for_source).tree(source.repo, commit.tree.sha)
        special_paths = T.let(Set.new, T::Set[String])
        base_tree.tree.each do |entry|
          # Submodules have type "commit"; symlinks have mode "120000"
          special_paths.add(entry.path) if entry.type == "commit" || entry.mode == "120000"

common/lib/dependabot/pull_request_updater/github.rb:249

• As in the PR creator, this method silently drops conflicting entries and continues, which can yield a branch update that 'succeeds' but doesn’t include all intended changes (or produces a no-op commit). Consider raising a specific error when entries are excluded so the update fails loudly and predictably.

        file_trees.reject do |entry|
          path = T.must(entry[:path])
          first_component = T.must(path.split("/").first)
          if path.include?("/") && special_paths.include?(first_component)
            Dependabot.logger.warn(
              "Excluding #{path} from create_tree — " \
              "conflicts with submodule/symlink '#{first_component}' in base tree"
            )
            true

common/lib/dependabot/pull_request_updater/github.rb:233

• filter_conflicting_entries is duplicated in both the updater and creator GitHub implementations. Extracting it to a shared helper would reduce maintenance overhead and help ensure fixes (e.g., nested path handling) are applied consistently.

      sig do
        params(file_trees: T::Array[T::Hash[Symbol, T.untyped]])
          .returns(T::Array[T::Hash[Symbol, T.untyped]])
      end
      def filter_conflicting_entries(file_trees)
        commit = T.unsafe(github_client_for_source).git_commit(source.repo, base_commit)
        base_tree = T.unsafe(github_client_for_source).tree(source.repo, commit.tree.sha)
        special_paths = T.let(Set.new, T::Set[String])

[Copilot]

filter_conflicting_entries only checks the first path component against special_paths, but special_paths is populated from a non-recursive root tree (tree(...) without recursive). This means conflicts with nested submodules/symlinks (e.g., a submodule at a/b) will not be detected, and updates under those paths can still produce invalid trees. Consider either fetching the tree recursively (and collecting full paths), or checking all path prefixes against special_paths derived by walking the tree hierarchy.

This issue also appears in the following locations of the same file:

• line 361
• line 350
• line 346

[Copilot]

Same issue as in the PR creator: special_paths is built from a non-recursive root tree and the filter only matches the first path component, so nested submodules/symlinks won't be detected and updates under them can still generate conflicting entries. Consider using a recursive tree fetch or prefix checking across the full path.

This issue also appears in the following locations of the same file:

• line 226
• line 241
• line 226