Skip to content

fix: include submodule files in file search - #4662

Closed
he-yufeng wants to merge 1 commit into
QwenLM:mainfrom
he-yufeng:fix/file-search-submodules
Closed

fix: include submodule files in file search#4662
he-yufeng wants to merge 1 commit into
QwenLM:mainfrom
he-yufeng:fix/file-search-submodules

Conversation

@he-yufeng

Copy link
Copy Markdown
Contributor

Summary

  • include tracked files inside submodules in the git-backed file crawler
  • keep the existing skip for the submodule gitlink itself
  • add a regression test that requires git ls-files --cached --recurse-submodules

Fixes #4568

To verify

  • npm exec -- vitest run packages/core/src/utils/filesearch/crawler.test.ts
  • npm exec -- eslint packages/core/src/utils/filesearch/crawler.ts packages/core/src/utils/filesearch/crawler.test.ts --max-warnings 0
  • npm exec -- prettier --check packages/core/src/utils/filesearch/crawler.ts packages/core/src/utils/filesearch/crawler.test.ts
  • npm run typecheck --workspace=@qwen-code/qwen-code-core

(args) => args.includes('ls-files') && args.includes('--cached'),
);
expect(cachedGitCall).toContain('--recurse-submodules');
expect(results).toContain('packages/nested/submodule-file.ts');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The test asserts that the submodule file is included in results, but doesn't assert the negative: that the submodule gitlink entry (packages/nested) itself is excluded. The mock data already sets up this scenario (S packages/nested), and the production code correctly filters S-status entries — but without a negative assertion, a regression in S-status filtering would go undetected.

Suggested change
expect(results).toContain('packages/nested/submodule-file.ts');
expect(cachedGitCall).toContain('--recurse-submodules');
expect(results).toContain('packages/nested/submodule-file.ts');
expect(results).not.toContain('packages/nested');

— qwen3.7-max via Qwen Code /review

@he-yufeng
he-yufeng force-pushed the fix/file-search-submodules branch from c31601d to 1b9239d Compare June 1, 2026 06:35
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Added the negative assertion from the review so the test now also verifies that the submodule gitlink entry itself is not returned.

Re-ran:

  • npm exec -- vitest run packages/core/src/utils/filesearch/crawler.test.ts -t "submodule"
  • git diff --check -- packages/core/src/utils/filesearch/crawler.test.ts

'--literal-pathspecs',
'ls-files',
'--cached',
'--recurse-submodules',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] --recurse-submodules is added to the --cached listing but git does not support it with --deleted (exits 128: fatal: ls-files --recurse-submodules unsupported mode). This creates a known asymmetry: a tracked-then-deleted file inside a submodule appears in the --cached --recurse-submodules output but cannot be filtered by deletedSet (populated from listDeletedTrackedFiles), resulting in a phantom entry in search results.

The practical impact is narrow (deleting tracked files inside submodules is uncommon), and the fix is non-trivial (would require per-submodule enumeration). Adding a brief code comment documenting this known limitation would help future maintainers who might otherwise attempt the same --recurse-submodules fix on the --deleted path and hit the git error.

Suggested change
'--recurse-submodules',
const trackedArgs = [
'--literal-pathspecs',
'ls-files',
'--cached',
// NOTE: --recurse-submodules is only supported for --cached/--stage.
// listDeletedTrackedFiles cannot use it, so a tracked file deleted
// inside a submodule may appear as a phantom entry in results.
'--recurse-submodules',
];

— qwen3.7-max via Qwen Code /review

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Closing this duplicate path in favor of #4596, which already has the fuller submodule fix and maintainer verification. Thanks for reviewing both.

@he-yufeng he-yufeng closed this Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: @ file completion shows submodule folder but no files inside it

2 participants