Skip to content

fix(core): recurse into submodule files when crawling git repos #4596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tanzhenxin merged 3 commits into from
Jun 8, 2026
Merged

fix(core): recurse into submodule files when crawling git repos #4596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21b5d5d
fix: recurse into submodules when crawling git files
he-yufeng May 28, 2026
c806c2a
fix: filter stale crawler index entries
he-yufeng May 29, 2026
cd5509f
fix(core): preserve quoted git paths in crawler
he-yufeng Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 173 additions & 0 deletions packages/core/src/utils/filesearch/crawler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,179 @@ describe('crawler', () => {
);
});

it('should preserve non-ASCII tracked paths from git output', async () => {
tmpDir = await createTmpDir({
'café.txt': '',
'文档.md': '',
plain: ['nested.txt'],
});
await initGitRepo(tmpDir);

const ignore = loadIgnoreRules({
projectRoot: tmpDir,
useGitignore: false,
useQwenignore: false,
ignoreDirs: [],
});

const results = await crawl({
crawlDirectory: tmpDir,
cwd: tmpDir,
ignore,
cache: false,
cacheTtl: 0,
});

expect(results).toEqual(
expect.arrayContaining(['café.txt', '文档.md', 'plain/nested.txt']),
);
});

it('should recurse into tracked submodules on the git path', async () => {
tmpDir = await createTmpDir({});
const parentRepo = path.join(tmpDir, 'parent');
const submoduleSource = path.join(tmpDir, 'submodule-source');

await fs.mkdir(parentRepo);
await fs.mkdir(submoduleSource);
await fs.writeFile(path.join(submoduleSource, 'inner.txt'), 'submodule');
await initGitRepo(submoduleSource);

await fs.writeFile(path.join(parentRepo, 'root.txt'), 'root');
await initGitRepo(parentRepo);
await runExecFile(
'git',
[
'-c',
'protocol.file.allow=always',
'submodule',
'add',
submoduleSource,
'vendor/lib',
],
parentRepo,
);
await runExecFile(
'git',
[
'-c',
'user.name=Qwen Test',
'-c',
'user.email=qwen-test@example.com',
'commit',
'--no-gpg-sign',
'-m',
'add submodule',
],
parentRepo,
);

const ignore = loadIgnoreRules({
projectRoot: parentRepo,
useGitignore: false,
useQwenignore: false,
ignoreDirs: [],
});

const results = await crawl({
crawlDirectory: parentRepo,
cwd: parentRepo,
ignore,
cache: false,
cacheTtl: 0,
});

expect(results).toContain('vendor/lib/inner.txt');

@wenshao wenshao Jun 4, 2026

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] This test only asserts that the submodule file appears in results, but doesn't verify that parent-repo files are also present. A regression that drops non-submodule tracked files while returning submodule files would pass undetected.

Suggested change
expect(results).toContain('vendor/lib/inner.txt');
expect(results).toContain('vendor/lib/inner.txt');
expect(results).toContain('root.txt');

— qwen3.7-max via Qwen Code /review

}, 15_000);

it('should skip missing tracked paths from submodule indexes', async () => {
tmpDir = await createTmpDir({});
await fs.mkdir(path.join(tmpDir, 'vendor', 'lib'), { recursive: true });
await fs.writeFile(path.join(tmpDir, 'vendor', 'lib', 'alive.txt'), '');

__setCommandRunnerForTests(async (command, args) => {
if (command !== 'git') {
return { success: false, lines: [] };
}
if (args.includes('rev-parse') && args.includes('--show-toplevel')) {
return { success: true, lines: [tmpDir] };
}
if (args.includes('ls-files') && args.includes('--others')) {
return { success: true, lines: [] };
}
if (args.includes('ls-files') && args.includes('--deleted')) {
return { success: true, lines: [] };
}
if (args.includes('ls-files') && args.includes('--cached')) {
return {
success: true,
lines: ['H vendor/lib/alive.txt', 'H vendor/lib/deleted.txt'],
};
}
return { success: false, lines: [] };
});

const ignore = loadIgnoreRules({
projectRoot: tmpDir,
useGitignore: false,
useQwenignore: false,
ignoreDirs: [],
});

const results = await crawl({
crawlDirectory: tmpDir,
cwd: tmpDir,
ignore,
cache: false,
cacheTtl: 0,
});

expect(results).toContain('vendor/lib/alive.txt');
expect(results).not.toContain('vendor/lib/deleted.txt');
});

it('should skip cached gitlink directories from uninitialized submodules', async () => {
tmpDir = await createTmpDir({});
await fs.mkdir(path.join(tmpDir, 'vendor', 'lib'), { recursive: true });

__setCommandRunnerForTests(async (command, args) => {
if (command !== 'git') {
return { success: false, lines: [] };
}
if (args.includes('rev-parse') && args.includes('--show-toplevel')) {
return { success: true, lines: [tmpDir] };
}
if (args.includes('ls-files') && args.includes('--others')) {
return { success: true, lines: [] };
}
if (args.includes('ls-files') && args.includes('--deleted')) {
return { success: true, lines: [] };
}
if (args.includes('ls-files') && args.includes('--cached')) {
return { success: true, lines: ['H vendor/lib'] };
}
return { success: false, lines: [] };
});

const ignore = loadIgnoreRules({
projectRoot: tmpDir,
useGitignore: false,
useQwenignore: false,
ignoreDirs: [],
});

const results = await crawl({
crawlDirectory: tmpDir,
cwd: tmpDir,
ignore,
cache: false,
cacheTtl: 0,
});

expect(results).not.toContain('vendor/lib');
expect(results).not.toContain('vendor/lib/');
});

it('should resolve the git root from a subdirectory crawl', async () => {
tmpDir = await createTmpDir({
src: ['file2.js'],
Expand Down
19 changes: 18 additions & 1 deletion packages/core/src/utils/filesearch/crawler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ function withSafeGitConfig(args: string[]): string[] {
'core.fsmonitor=false',
'-c',
'core.untrackedCache=false',
'-c',
'core.quotePath=false',
...args,
];
}
Expand Down Expand Up @@ -1036,7 +1038,12 @@ async function crawlWithGitLsFiles(

// Avoid `-z` with `-t`: record shape for `ls-files -t` + `-z` is not stable across Git
// versions; newline-delimited output is fine here (index paths cannot contain newlines).
const trackedArgs = ['--literal-pathspecs', 'ls-files', '--cached'];
const trackedArgs = [
'--literal-pathspecs',

@wenshao wenshao May 28, 2026

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.

[Critical] git ls-files --deleted --recurse-submodules is unsupported by git (fatal: "unsupported mode"). This creates an asymmetry: when a tracked file inside an initialized submodule is deleted from the working tree, --cached --recurse-submodules still lists it (from the submodule's index), but listDeletedTrackedFiles (--deleted) cannot detect the deletion. The deletedSet check in processTrackedFile therefore does not filter it, and the crawler returns a phantom path that doesn't exist on disk.

Impact: File search results include ghost entries for deleted submodule files. Downstream consumers (read file, @-mention completion) will get ENOENT errors.

Suggested change
'--literal-pathspecs',
const trackedArgs = [
'--literal-pathspecs',
'ls-files',
'--cached',
'--recurse-submodules',
];
// NOTE: git does not support `--deleted --recurse-submodules` (fatal error).
// Deleted files inside submodules won't appear in deletedSet.
// Consider adding an fs.access() guard for paths under submodule mount points,
// or use `git submodule foreach --recursive 'git ls-files -z --deleted'`
// to collect submodule-internal deleted files.

Also: When a submodule is registered but not initialized (common after plain git clone), --cached --recurse-submodules -t emits the gitlink mount point (e.g., H vendor/lib) as a cached entry. processTrackedFile only skips status S, but gitlinks appear as H — so a directory path ends up in the file list. Consider filtering gitlink entries or adding a stat guard.

— qwen3.7-max via Qwen Code /review

'ls-files',
'--cached',

@wenshao wenshao Jun 4, 2026

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] If any submodule is uninitialized or has a broken .git reference, git ls-files --cached --recurse-submodules exits non-zero and the entire tracked-file listing fails (line ~1141), falling back to ripgrep — which silently loses git-tracking semantics (force-added files matching .gitignore are dropped, untracked files appear).

Consider retrying without --recurse-submodules before returning failure, so a broken submodule only loses submodule files rather than degrading the entire parent repo listing:

if (!trackedResult.success) {
  // Retry without --recurse-submodules: broken submodule states can cause
  // the recursive listing to fail entirely.
  const fallbackArgs = trackedArgs.filter(a => a !== '--recurse-submodules');
  const fallbackResult = await commandRunner('git', withSafeGitConfig(fallbackArgs), ...);
  if (!fallbackResult.success) {
    return { success: false, files: [], gitRepoListingFailed: true };
  }
  // ... process fallbackResult
}

— qwen3.7-max via Qwen Code /review

'--recurse-submodules',

@wenshao wenshao Jun 7, 2026

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 --cached only, not to --others (line 852) or --deleted (line 878). The asymmetry is correct — --others and --deleted operate on the working tree, not submodule indexes — but no comment explains why. A future maintainer might "harmonize" the three calls by adding the flag everywhere, which would break behavior: --deleted --recurse-submodules would emit gitlink directory entries that the deletedSet filter cannot handle.

Suggested change
'--recurse-submodules',
// --recurse-submodules is intentionally scoped to --cached only:
// --others would surface untracked submodule internals (policy choice),
// and --deleted would emit gitlink entries that deletedSet cannot handle.
'--recurse-submodules',

— qwen3.7-max via Qwen Code /review

];
trackedArgs.push('-t');
if (relativeToGitRoot && relativeToGitRoot !== '.') {
trackedArgs.push(relativeToGitRoot);
Expand Down Expand Up @@ -1077,6 +1084,16 @@ async function crawlWithGitLsFiles(
return true;
}

let stat: fs.Stats;
try {
stat = fs.lstatSync(path.join(gitRoot, ...normalizedFile.split('/')));

@BZ-D BZ-D May 31, 2026

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.

nit: lstatSync is called for every tracked file in the streaming onLine callback, not just submodule-related entries. In large repos (50K+ files) this adds ~0.5–1s of synchronous I/O that blocks the event loop.

A lighter alternative: parse .gitmodules (or git config --file .gitmodules -l) once to collect known submodule paths, then only stat entries whose prefix matches a submodule path. That narrows the stat surface to submodule root entries instead of the entire file list.

Not blocking — the correctness is fine, just something to keep in mind if crawl latency regresses on large monorepos.

@wenshao wenshao Jun 7, 2026

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] This lstatSync + isDirectory() guard filters gitlink entries (mode 160000) that --recurse-submodules emits for uninitialized submodules. The guard's purpose is non-obvious — a future maintainer optimizing hot-path I/O (one synchronous stat per tracked file) might remove it as seemingly dead code, re-introducing the bug.

Consider adding a comment tying the guard to its reason:

// --recurse-submodules causes ls-files --cached to emit gitlink entries
// (directory placeholders) for uninitialized submodules. Skip those here.

Also, path.join(gitRoot, ...normalizedFile.split('/')) can be simplified to path.join(gitRoot, normalizedFile) — path.join already handles / separators on all platforms, and every other path.join call in this file uses direct string args.

— qwen3.7-max via Qwen Code /review

} catch {
return true;
}
if (stat.isDirectory()) {
return true;
}

if (
relativeToGitRoot &&
relativeToGitRoot !== '.' &&
Expand Down
Loading