fix(core): make read_file respect git-ignore settings for consistency with list_directory - #6154
fix(core): make read_file respect git-ignore settings for consistency with list_directory#6154Alex-ai-future wants to merge 5 commits into
Conversation
… with list_directory read_file previously only checked .qwenignore patterns and ignored .gitignore rules entirely, while list_directory respected both. This makes read_file respect the same git-ignore and qwen-ignore settings as list_directory, using the same configuration options and per-call override pattern. Added file_filtering_options parameter to read_file (matching list_directory's API), allowing per-call override of respect_git_ignore and respect_qwen_ignore. Updated validation to read config.getFileFilteringOptions() as the default, then apply per-call overrides from the tool parameters. Uses the unified fileService.shouldIgnoreFile() API instead of the previous shouldQwenIgnoreFile()-only check. Closes QwenLM#6119 Signed-off-by: Alex <alex.tech.lab@outlook.com>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x).
The workflow fixes (--method GET additions) are correct bugfixes, and the file_filtering_options addition to read_file is a clean, well-tested consistency improvement with list_directory. Build and all 69 tests pass locally.
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Missing test for respect_qwen_ignore per-call override
The new file_filtering_options parameter exposes both respect_git_ignore and respect_qwen_ignore, but only respect_git_ignore has per-call override tests. The respect_qwen_ignore: false override path is completely untested — if the ?? fallback had a bug (e.g., always resolving to the config value regardless of the per-call override), no test would catch it.
Consider adding a test in the existing qwen-ignore describe block:
it('should allow reading qwen-ignored files when respect_qwen_ignore is false via per-call override', () => {
const ignoredFilePath = path.join(tempRootDir, 'cursor-secret.txt');
const invocation = tool.build({
file_path: ignoredFilePath,
file_filtering_options: { respect_qwen_ignore: false },
});
expect(typeof invocation).not.toBe('string');
});— qwen3.7-max via Qwen Code /review
| await fsp.writeFile( | ||
| path.join(tempRootDir, '.gitignore'), | ||
| ['secret.env', 'ignored-dir/'].join('\n'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Missing test for directory-level gitignore patterns
The .gitignore setup includes ignored-dir/ but no test reads a file from inside that directory. Directory-level gitignore patterns can behave differently from file-level patterns (the ignore library checks the full path including parent directory components). A bug where directory-pattern matches fail while file-pattern matches work would go undetected.
Consider adding:
it('should throw error for files inside a git-ignored directory', () => {
const ignoredDirPath = path.join(tempRootDir, 'ignored-dir');
const ignoredFilePath = path.join(ignoredDirPath, 'data.txt');
expect(() => tool.build({ file_path: ignoredFilePath }))
.toThrow(/\.gitignore/);
});— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
This PR adds file_filtering_options (with respect_git_ignore and respect_qwen_ignore) to the read_file tool, bringing it in line with list_directory which already honors these settings. The implementation correctly delegates to the existing shouldIgnoreFile / shouldGitIgnoreFile methods on the file service, with per-call overrides falling back to config defaults via nullish coalescing. Tests cover the key paths: git-ignored files rejected, non-ignored files allowed, and per-call override toggling. Looks correct.
— qwen3-coder via Qwen Code /review
|
Thanks for the careful writeup and the repro in #6119 — the asymmetry you spotted is real. But I think this PR resolves it in the wrong direction, and I'd hold off on merging as-is. The core problem:
|
…eedback read_file should not block git-excluded files by default. .gitignore means 'do not commit', not 'do not read'. Default respect_git_ignore to false, keeping per-call override as opt-in via file_filtering_options. respect_qwen_ignore still follows config default. Addresses review feedback on PR QwenLM#6154. Signed-off-by: Alex <alex.tech.lab@outlook.com>
| if (fileService.shouldQwenIgnoreFile(params.file_path)) { | ||
| const configOpts = this.config.getFileFilteringOptions(); | ||
| const respectGitIgnore = | ||
| params.file_filtering_options?.respect_git_ignore ?? false; |
There was a problem hiding this comment.
[Critical] respectGitIgnore falls back to hardcoded false, ignoring the user's config setting and the global default.
Every other tool in the codebase uses a 3-level fallback chain: per-call ?? config ?? DEFAULT_FILE_FILTERING_OPTIONS. For example, ls.ts:
respectGitIgnore:
this.params.file_filtering_options?.respect_git_ignore ??
this.config.getFileFilteringOptions().respectGitIgnore ??
DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, // = trueHere, configOpts is fetched on the line above but configOpts.respectGitIgnore is never read — it's dead code. Since DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore is true, this means list_directory blocks gitignored files by default but read_file silently reads them. This also contradicts the schema description at line 445 which says "Defaults to the value from settings."
The test at read-file.test.ts:1250 ("should allow reading git-ignored files by default") encodes this bug: the mock config has respectGitIgnore: true yet the test asserts the file is readable, which only passes because the config is never consulted.
| params.file_filtering_options?.respect_git_ignore ?? false; | |
| const respectGitIgnore = | |
| params.file_filtering_options?.respect_git_ignore ?? | |
| configOpts.respectGitIgnore ?? | |
| DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore; |
Also add import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; at the top of the file (matching ls.ts and glob.ts).
The test "should allow reading git-ignored files by default" must also be updated — with the fix, the default config (respectGitIgnore: true) should block gitignored files:
it('should block git-ignored files by default when config respects git-ignore', () => {
const ignoredFilePath = path.join(tempRootDir, 'secret.env');
expect(() => tool.build({ file_path: ignoredFilePath })).toThrow(/\.gitignore/);
});— qwen3.7-max via Qwen Code /review
| const respectGitIgnore = | ||
| params.file_filtering_options?.respect_git_ignore ?? false; | ||
| const respectQwenIgnore = | ||
| params.file_filtering_options?.respect_qwen_ignore ?? |
There was a problem hiding this comment.
[Suggestion] respectQwenIgnore uses a 2-level fallback (per-call ?? config) and omits the third-level DEFAULT_FILE_FILTERING_OPTIONS.respectQwenIgnore safety net that ls.ts includes. While getFileFilteringOptions() currently always returns a complete object, if it ever returned a partial (e.g., from a new config backend), respectQwenIgnore would silently become undefined (falsy), disabling qwen-ignore filtering with no warning.
| params.file_filtering_options?.respect_qwen_ignore ?? | |
| const respectQwenIgnore = | |
| params.file_filtering_options?.respect_qwen_ignore ?? | |
| configOpts.respectQwenIgnore ?? | |
| DEFAULT_FILE_FILTERING_OPTIONS.respectQwenIgnore; |
— qwen3.7-max via Qwen Code /review
| fileService.shouldGitIgnoreFile(params.file_path) | ||
| ) { | ||
| return `File path '${filePath}' is ignored by .gitignore pattern(s).`; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The error-message routing calls shouldGitIgnoreFile a second time after shouldIgnoreFile already matched internally. This double-check pattern duplicates work and can misattribute the cause: if a file is both git-ignored and qwen-ignored, the user only sees the .gitignore error and never learns about the .qwenignore match. More importantly, the two non-atomic calls could diverge if either method is refactored.
Consider replacing shouldIgnoreFile + re-check with sequential individual checks:
if (respectGitIgnore && fileService.shouldGitIgnoreFile(params.file_path)) {
return `File path '${filePath}' is ignored by .gitignore pattern(s).`;
}
if (respectQwenIgnore && fileService.shouldQwenIgnoreFile(params.file_path)) {
return `File path '${filePath}' is ignored by ${fileService.getQwenIgnoreFileDisplayForPath(params.file_path)} pattern(s).`;
}— qwen3.7-max via Qwen Code /review
…dling - Add comment explaining why respect_git_ignore defaults to false (.gitignore is for version control, not access control) - Fix respect_qwen_ignore to use 3-level fallback chain: per-call ?? config ?? DEFAULT_FILE_FILTERING_OPTIONS - Replace shouldIgnoreFile + re-check with sequential individual checks to avoid redundant double-calls and provide accurate error messages - Fix schema description to match actual default (false, not 'from settings') Addresses review feedback on PR QwenLM#6154. Signed-off-by: Alex <alex.tech.lab@outlook.com>
| file_filtering_options?: { | ||
| /** | ||
| * Whether to respect .gitignore patterns when reading files. | ||
| * Only available in git repositories. Defaults to settings value. |
There was a problem hiding this comment.
[Suggestion] JSDoc for respect_git_ignore says "Defaults to settings value." but the implementation at line 525 hardcodes ?? false, never consulting the settings value. The JSON schema description (line 446) correctly states "Defaults to false because .gitignore is for version control, not access control."
The TypeScript interface JSDoc and the JSON schema give opposite defaults. A future engineer adding file_filtering_options to another tool (e.g., notebook-edit) will read the interface as the contract and implement the wrong default.
| * Only available in git repositories. Defaults to settings value. | |
| * Whether to respect .gitignore patterns when reading files. | |
| * Only available in git repositories. Defaults to false (not the settings value). | |
| * .gitignore means "do not commit", not "do not read". |
— qwen3.7-max via Qwen Code /review
| }, | ||
| respect_qwen_ignore: { | ||
| description: | ||
| 'Optional: Whether to respect .qwenignore and configured custom Qwen ignore file patterns when reading files. Defaults to the value from settings.', |
There was a problem hiding this comment.
[Suggestion] .qwenignore was previously unconditionally enforced by read_file (old code: if (fileService.shouldQwenIgnoreFile(...))). This PR makes it configurable per-call, allowing the LLM to pass respect_qwen_ignore: false and bypass .qwenignore protections.
While this matches the ls.ts pattern, read_file is the tool most likely to be targeted for reading sensitive files. Consider whether the per-call override should only be allowed to tighten protection (force true), never relax it. Alternatively, document this as an accepted tradeoff for API consistency.
— qwen3.7-max via Qwen Code /review
| expect(typeof invocation).not.toBe('string'); | ||
| }); | ||
|
|
||
| it('should allow reading qwen-ignored files when respect_qwen_ignore is false via per-call override', () => { |
There was a problem hiding this comment.
[Suggestion] All mock configs in this test file return respectQwenIgnore: true from getFileFilteringOptions() (lines 59, 1101, 1186). Since DEFAULT_FILE_FILTERING_OPTIONS.respectQwenIgnore is also true, the middle branch of the 3-level fallback (configOpts.respectQwenIgnore) can never be distinguished from the default. A regression that silently skips the config lookup would go undetected.
Consider adding a test with a mock config returning respectQwenIgnore: false, asserting that a qwen-ignored file is readable without per-call override. This pins the config-level fallback.
— qwen3.7-max via Qwen Code /review
|
这个 PR 的方向不对。为避免自动化程序错误合入，先关闭。 |
5 participants
title: "fix(core): make read_file respect git-ignore settings for consistency with list_directory"
What this PR does
read_filepreviously only checked.qwenignorepatterns and ignored.gitignorerules entirely, whilelist_directoryrespected both. This PR makesread_filerespect the same git-ignore and qwen-ignore settings aslist_directory, using the same configuration options and per-call override pattern.Specifically:
file_filtering_optionsparameter toread_file(matchinglist_directory's API), allowing per-call override ofrespect_git_ignoreandrespect_qwen_ignoreconfig.getFileFilteringOptions()as the default, then apply per-call overrides from the tool parametersfileService.shouldIgnoreFile()API instead of the previousshouldQwenIgnoreFile()-only checkWhy it's needed
The inconsistency between
list_directoryandread_filecreated a confusing state:list_directory(filtered out)@pathin TUI (silently skipped)find),read_filewould successfully read the fileThis meant tools that work together in the same workflow behaved inconsistently — one blocked access to git-excluded files while another allowed it. Now both tools respect the same settings, and users can control the behavior globally via settings or per-call via
file_filtering_options.Reviewer Test Plan
How to verify
Run the read-file tests and confirm the new git-ignore tests pass:
Verify the new tests under
with .gitignore:should throw error if path is ignored by .gitignore when respectGitIgnore is true— confirms git-ignore is enforcedshould allow reading git-ignored files when respectGitIgnore is false— confirms per-call override worksshould allow reading non-git-ignored files— confirms normal files are unaffectedshould respect per-call file_filtering_options to override git-ignore— confirms parameter-level override of config settingsEvidence (Before & After)
Before:
read_filevalidation only calledshouldQwenIgnoreFile(), never checking git-ignore. Git-excluded files could be read if the path was known.After:
read_filecallsshouldIgnoreFile()with bothrespectGitIgnoreandrespectQwenIgnorefrom config, with per-call override support viafile_filtering_options.Tested on
Environment (optional)
N/A — unit tests only.
Risk & Scope
notebook-edit.tshas a similar qwen-ignore-only check — deferred to a follow-up.file_filtering_optionsinherit the config default (same as before for qwen-ignore, now also respects git-ignore).Linked Issues
Closes #6119
中文说明
这个 PR 做了什么
read_file之前只检查.qwenignore规则，完全不遵守.gitignore，而list_directory两者都遵守。这个 PR 让read_file和list_directory使用相同的 git-ignore 和 qwen-ignore 设置。具体修改：
read_file添加了file_filtering_options参数（与list_directoryAPI 一致），支持每次调用时覆盖respect_git_ignore和respect_qwen_ignoreconfig.getFileFilteringOptions()作为默认值，然后用工具参数覆盖fileService.shouldIgnoreFile()API，替代之前只检查shouldQwenIgnoreFile()的逻辑为什么需要
两个工具行为不一致导致困惑的状态：
list_directory发现 git-excluded 文件（被过滤）@path引用（被静默跳过）find），read_file却能正常读取现在两个工具遵守相同的设置，用户可以通过全局设置控制，也可以在每次调用时通过
file_filtering_options覆盖。验证方式
69 个测试全部通过，包括 4 个新增的 git-ignore 测试。
关联 Issue
Closes #6119