fix(core): reject unverifiable validated read inodes - #9857
Conversation
Fail closed when validated @-file reads encounter zero inode identities so dev/ino comparisons cannot silently accept unrelated files on filesystems that do not expose stable inode numbers. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the fix, @AaronZ345 — the direction matches #8227, but the PR description doesn't follow this repo's template, so it can't enter review as-is.
- Missing required sections — the body needs the headings from the PR template:
What this PR does,Why it's needed,Reviewer Test Plan(withHow to verify,Evidence (Before & After),Tested on),Risk & Scope,Linked Issues, plus the Chinese translation in<details>. - Current body uses a generic
Summary/Test planstructure instead. Please edit the PR description to fill in the template — the test commands you already listed fit directly underHow to verify, andFixes #8227belongs underLinked Issues. - This is a formatting gate, not a verdict on the code. Once the description follows the template, re-trigger with
@qwen-code /triageand the run will pick it up from the top.
中文说明
感谢这个修复，@AaronZ345 —— 方向与 #8227 一致，但 PR 描述没有遵循本仓库的模板，暂时无法进入评审。
- 缺少必需章节 —— 正文需要使用 PR 模板 中的标题：
What this PR does、Why it's needed、Reviewer Test Plan（含How to verify、Evidence (Before & After)、Tested on）、Risk & Scope、Linked Issues，以及<details>中的中文翻译。 - 当前正文使用的是通用的
Summary/Test plan结构。请编辑 PR 描述按模板填写——已列出的测试命令可以直接放进How to verify，Fixes #8227应放在Linked Issues下。 - 这只是格式关卡，不是对代码的结论。描述按模板更新后，用
@qwen-code /triage重新触发即可从头评审。
— Qwen Code · qwen3.8-max
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6a": executing readManyFiles.test.ts under vitest — the worktree has no node_modules / dist , so running requires a full npm ci + npm run build ; I relied on s….
— qwen3.8-max via Qwen Code /review (v0.22.0)
| @@ -159,6 +160,9 @@ export async function readManyFiles( | |||
| const displayPath = displayPaths?.get(fullPath) ?? fullPath; | |||
| const validatedIdentity = validatedPathIdentities?.get(fullPath); | |||
| if (validatedPathIdentities && !validatedIdentity) continue; | |||
| if (validatedIdentity && !hasVerifiableInode(validatedIdentity.ino)) { | |||
There was a problem hiding this comment.
[Suggestion] Fixes #8227 overclaims the issue's scope. Issue #8227's Proposal has three items: (1) run the atCommandProcessor / Session / readManyFiles validated-read suites on a Windows runner (junctions + NTFS symlinks), (2) decide the posture for identity checks on ino: 0 filesystems — fail closed or document the residual risk, (3) un-skip whichever regression tests can run under Windows semantics. This diff implements only proposal 2 (the fail-closed branch); the O_NOFOLLOW collapse named in the issue title, the adjacent ino: 0 sites named in the issue thread (FileReadCache.inodeKey, workspace-file-system.ts assertSameFile, session-transcript-reader.ts), and proposals 1 and 3 are untouched — the PR body itself concedes "Not validated / out of scope: Windows native filesystem manual testing".
The cost is tracking: merging with Fixes auto-closes #8227, and the still-open Windows-runner validation and un-skipping work (both explicitly listed in the issue body) lose their tracking issue and remain tracked nowhere. Consider changing the keyword to Part of #8227 and opening follow-up issues for proposals 1 and 3, or expanding this PR to cover them before closing the issue. Note the ino: 0 fail-closed posture was recorded as a maintainer call in the issue thread, so a maintainer confirmation is worth obtaining either way.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const stats = await source.stat(); | ||
| if ( | ||
| !stats.isFile() || | ||
| !hasVerifiableInode(stats.ino) || |
There was a problem hiding this comment.
[Suggestion] This handle-bound validated-identity predicate is duplicated verbatim in snapshotValidatedFile (~lines 373-379), and this PR pasted the same new hasVerifiableInode clause into both copies in lockstep; a third variant lives in matchesValidatedPathIdentity (~line 342). The drift cost is demonstrated by this very change: the next change to identity semantics (another unverifiable-inode shape, a new comparison rule) must find and edit every copy, and missing one silently leaves either the text-handle read path or the binary snapshot path with a weaker check than its sibling — precisely the fail-closed asymmetry this change exists to eliminate. (matchesValidatedPathIdentity cannot be the shared helper for the two fd-stat sites — it re-resolves via realpath and re-stats by path instead of trusting the already-opened handle's stats.)
One small leaf predicate would close it:
function statsMatchValidatedIdentity(
stats: fs.Stats,
expected: ReadManyFilesPathIdentity,
): boolean {
return (
stats.isFile() &&
hasVerifiableInode(stats.ino) &&
stats.dev === expected.dev &&
stats.ino === expected.ino
);
}— qwen3.8-max via Qwen Code /review (v0.22.0)
| @@ -159,6 +160,9 @@ export async function readManyFiles( | |||
| const displayPath = displayPaths?.get(fullPath) ?? fullPath; | |||
| const validatedIdentity = validatedPathIdentities?.get(fullPath); | |||
| if (validatedPathIdentities && !validatedIdentity) continue; | |||
| if (validatedIdentity && !hasVerifiableInode(validatedIdentity.ino)) { | |||
There was a problem hiding this comment.
[Suggestion] On inode-0 filesystems (FAT/exFAT, some SMB mounts — the class file-identity.ts documents), this guard silently drops every validated @-read: a bare continue, no log, no error entry in result.files, no user-visible reason. Before this PR those reads succeeded (0 === 0 matched — verified against HEAD~1), so this is a newly reachable silent failure with zero diagnostics. Both production callers (atCommandProcessor.ts, Session.ts) unconditionally record { dev, ino } from stat and always pass the map, so there is no fallback; on such mounts 100% of validated reads are dropped, permanently disabling @-mentions.
The user experience is an attachment vanishing: result.files stays empty, the TUI renders no "Read File" card, and the model only receives the generic "No files matching the criteria were found or all were skipped." — indistinguishable from a workspace-bounds bug, an ignore-rule bug, or a path-resolution bug. atCommandProcessor already renders FileReadInfo.error entries as visible failed reads, but a dropped path produces no FileReadInfo at all (and Session.ts consumes only contentParts, so the ACP path has no per-file rendering). The fail-closed rationale justifies the drop, not the silence. Surfacing a skip reason — e.g. pushing a FileReadInfo with error: 'inode identity unverifiable on this filesystem (ino=0)', or exposing skipped paths on the result — would make the failure self-describing. (Confirmed by running the new regression test: it asserts content excludes the file and result.files has length 0.)
— qwen3.8-max via Qwen Code /review (v0.22.0)
Report validated reads with unverifiable inode identity as explicit file errors and share the file identity predicate across validated read paths. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1
Fixes #8227overclaims the issue's scope — already reported (comment 3841306407)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1a": running the packages/core vitest suite / typecheck for readManyFiles.test.ts — the review worktree has no node_modules installed, so compile/run verification co….
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1a"：running the packages/core vitest suite / typecheck for readManyFiles.test.ts — the review worktree has no node_modules installed, so compile/run verification co…。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| contentParts.push(...errorParts); | ||
| files.push(info); |
There was a problem hiding this comment.
[Suggestion] This new error branch emits before the seenFiles dedup, while the success-read branch sits behind it, so repeated @-mentions of the same path emit one error block per mention. atCommandProcessor pushes one path entry per @-mention with no dedup (the ACP session dedups via a Set), so duplicate mentions flow through to readManyFiles. Before this commit, duplicate mentions on inode-0 volumes matched vacuously and were read once with seenFiles absorbing the duplicate — the duplication was invisible; the new branch emits per iteration.
Concrete trigger: submitting @notes.txt @notes.txt in the interactive TUI on a FAT/exFAT/SMB workspace (every stat reports ino: 0 — the exact filesystem class this PR targets) makes both loop iterations hit this guard, so contentParts receives two identical "Validated file identity is unavailable" blocks and files gets two entries, rendering two identical failed-read tool cards.
Verified by probe on the PR source: paths: ['dup.txt', 'dup.txt'] with identity { dev, ino: 0 } produces {"errorBlocksInContent":2,"filesLength":2,"filesWithError":2}; gating the emission on seenFiles in a scratch tree yields {"errorBlocksInContent":1,"filesLength":1} with all 53 tests still green.
| contentParts.push(...errorParts); | |
| files.push(info); | |
| if (!seenFiles.has(fullPath)) { | |
| seenFiles.add(fullPath); | |
| contentParts.push(...errorParts); | |
| files.push(info); | |
| } |
中文说明
[建议] 这个新的错误分支在 seenFiles 去重之前就输出结果，而成功读取分支在去重之后，因此对同一路径的重复 @ 提及会为每次提及输出一个错误块。atCommandProcessor 按每个 @ 提及推入一个路径条目且不去重（ACP session 用 Set 去重），所以重复提及会直接进入 readManyFiles。在此提交之前，inode-0 卷上的重复提及会空洞匹配并被读取一次，由 seenFiles 吸收重复——重复是不可见的；新分支则按迭代次数输出。
具体触发：在 FAT/exFAT/SMB 工作区（每次 stat 都报告 ino: 0——正是本 PR 针对的文件系统类别）的交互式 TUI 中提交 @notes.txt @notes.txt，两次循环迭代都会命中这个守卫，contentParts 会收到两个相同的 "Validated file identity is unavailable" 块，files 得到两个条目，渲染出两张相同的读取失败工具卡片。
已通过探针在 PR 源码上验证：paths: ['dup.txt', 'dup.txt'] 且身份为 { dev, ino: 0 } 时产生 {"errorBlocksInContent":2,"filesLength":2,"filesWithError":2}；在临时树中用 seenFiles 门控输出后得到 {"errorBlocksInContent":1,"filesLength":1}，53 个测试全部仍然通过。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| function createFileReadErrorResult( | ||
| displayPath: string, | ||
| errorMessage: string, | ||
| ): { contentParts: Part[]; info: FileReadInfo } { |
There was a problem hiding this comment.
[Suggestion] The new helper duplicates byte-for-byte the error-result block already constructed inline in this same file (~lines 236-247, the readValidatedTextFileContent catch block): same \nContent from ${displayPath}:\n prefix part, same Error reading ${displayPath}: ${errorMessage} text, same FileReadInfo with isDirectory: false and error. The cost is future drift: a change to the failed-read shape (rewording the prefix, adding a FileReadInfo field for error entries) reaches this helper — which the new regression test pins — but not the inline catch block, so the "identity unverifiable" error and the "read threw" error then render differently for the same class of failure, invisibly to tests that exercise only one path (atCommandProcessor's file.error tool-card rendering would diverge between the two paths).
Route the catch block through the helper too (keeping the abort rethrow and the errorMessage binding above it):
readResult = createFileReadErrorResult(displayPath, errorMessage);中文说明
[建议] 新 helper 与同一文件中已有的内联错误结果构造（约 236-247 行，readValidatedTextFileContent 的 catch 块）逐字节重复：相同的 \nContent from ${displayPath}:\n 前缀部分、相同的 Error reading ${displayPath}: ${errorMessage} 文本、相同的带 isDirectory: false 和 error 的 FileReadInfo。代价是未来的漂移：对读取失败形状的修改（改写前缀、为错误条目新增 FileReadInfo 字段）会到达这个被新回归测试钉住的 helper，但到不了内联 catch 块，于是 "identity unverifiable" 错误和 "read threw" 错误会对同一类失败呈现不同的渲染，而只覆盖其中一条路径的测试无法发现（atCommandProcessor 的 file.error 工具卡片渲染会在两条路径之间出现分歧）。
建议把 catch 块也改为调用该 helper（保留前面的 abort 重新抛出和 errorMessage 绑定）：
readResult = createFileReadErrorResult(displayPath, errorMessage);— qwen3.8-max via Qwen Code /review (v0.22.0)
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) is merge_group-only (never runs at the PR stage) and its E2E suite did not run locally.
Not explored to full depth (tool budget reached): "agent 5": executing packages/core/src/tools/readManyFiles.test.ts to confirm the new tests are green — the review worktree has no node_modules or built dist/ , and a….
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) is merge_group-only (never runs at the PR stage) and its E2E suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 5"：executing packages/core/src/tools/readManyFiles.test.ts to confirm the new tests are green — the review worktree has no node_modules or built dist/ , and a…。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it('deduplicates unverifiable validated inode errors for repeated paths', async () => { | ||
| const { relativePath, absolutePath } = await createTestFile( | ||
| 'zero-inode-duplicate.txt', |
There was a problem hiding this comment.
[Suggestion] The two new zero-inode tests paste the same ~28-line fs.stat/fs.open mocking block verbatim — this test's setup is byte-identical to the one in 'surfaces an error when validated inode identity is unverifiable' (lines ~191-214).
The cost is maintenance drift, and it is measurable today: these spies target exactly the object production calls (fs.promises), but they are never invoked, because the new early guard in readManyFiles.ts fires on the map's ino: 0 before any fs access. So one copy can silently lose its precondition with nothing noticing — deleting this test's mocking block in a scratch tree left the whole file green (52/52), including this test's toHaveLength(1) dedup assertion. Any future edit to the zero-inode precondition (e.g. also mocking lstat, handling bigint stats) must now be made in two places; if one copy rots, the test blesses the very regression it was written to catch.
Extract a shared helper next to createTestFile and call it from both tests, e.g.:
function mockZeroInodeForPath(absolutePath: string): { restore(): void } {
const statSpy = vi.spyOn(fs, 'stat');
const openSpy = vi.spyOn(fs, 'open');
// move the shared mockImplementation blocks here
return {
restore: () => {
statSpy.mockRestore();
openSpy.mockRestore();
},
};
}(checked in a scratch tree: with the helper extracted, all 52 tests still pass)
中文说明
两个新的 zero-inode 测试逐字粘贴了同一段约 28 行的 fs.stat/fs.open mock 代码块——本测试的 setup 与 'surfaces an error when validated inode identity is unverifiable'（约 191-214 行）中的完全相同。
代价是维护上的漂移，而且现在就可以度量：这些 spy 恰好挂在生产代码调用的对象（fs.promises）上，但从未被调用，因为 readManyFiles.ts 里新的早期守卫会在任何 fs 访问之前，基于 map 中的 ino: 0 提前触发。因此其中一个副本即使悄悄丢失前置条件，也不会有任何测试察觉——在临时树中删掉本测试的 mock 代码块后，整个文件仍然 52/52 全绿，包括本测试的 toHaveLength(1) 去重断言。将来任何对 zero-inode 前置条件的修改（例如同时 mock lstat、处理 bigint stats）都必须在两处同步；若其中一个副本腐化，测试反而会放行它本应捕获的回归。
在 createTestFile 旁提取一个共享 helper 并让两个测试都调用它（示例见上方英文代码块）。已在临时树中验证：提取 helper 后 52 个测试仍全部通过。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Consolidate duplicated zero-inode test setup so validated read diagnostics stay covered without copy-pasted filesystem spies. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 2": executing readManyFiles.test.ts at the reviewed commit — the worktree has no node_modules and npm ci + monorepo build to satisfy vitest's globalSetup prereq….
中文说明
未发现问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 2"：executing readManyFiles.test.ts at the reviewed commit — the worktree has no node_modules and npm ci + monorepo build to satisfy vitest's globalSetup prereq…。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Local maintainer verification round —
|
| Cell | base (6a21c43) | head (4310865) |
|---|---|---|
zero — identity {dev, ino:0}, read-time stat also 0 |
content included (vacuous match), no error | content excluded, visible error Validated file identity is unavailable on this filesystem (inode is 0). |
dup — same path twice |
marker ×1, no error | error ×exactly 1, content excluded |
normal — real APFS identity |
content ✓ | byte-identical to base |
mismatch — file swapped before read |
silent drop | byte-identical |
statszero — expected ≠ 0, current stat 0 |
silent drop | byte-identical |
openerror — chmod 000 before read |
EACCES surfaced | byte-identical (refactor proven behavior-preserving) |
Cell totals: base 19/19, head 25/25 (head includes 4 cross-tree byte-identity assertions). The failure is not silent on the TUI path: files[].error maps to ToolCallStatus.Error with a Failed to read … display.
Mutation matrix (vacuity + positive control)
| Mutation | Result | Verdict |
|---|---|---|
| M1 — delete the zero-inode pre-check | 50/52, the 2 new tests red | pinned — both load-bearing |
M2 — fine: delete only seenFiles.add in the pre-check |
51/52, dedup test red | pinned precisely |
M3 — positive control: drop stats.ino === expected.ino |
49/52, 3 drift tests red | pinned — suite can go red |
M4 — drop hasVerifiableInode(stats.ino) |
52/52 green | survivor → defense-in-depth, not load-bearing |
Reviewer Test Plan + gates
- read-many-files tests: head 52/52 (base 50/50 — delta +2, zero failures either side).
- Core typecheck (
tsc --noEmit): exit 0. - Prettier on the 5 touched files: all pass; gate proven live by a planted violation that was flagged and removed.
Findings
No blocking findings. Completeness notes: (1) the hasVerifiableInode(stats.ino) clause inside statsMatchValidatedIdentity cannot change any observable outcome today (all callers sit behind the expected-side pre-check) — M4 survivor, reasonable defense-in-depth, keep-or-drop is a style call; (2) other dev:ino consumers in the codebase (sessionArtifacts.ts, conversation-runtime-ownership.ts, discovery.ts, same-file.ts, settings-cache.ts) are change-detection, not approval gates, and remain vacuous-on-zero-inode by design — out of scope here, listed to avoid over-reading this PR's coverage.
Not covered
No real FAT/exFAT/SMB volume (boundary simulation only); Windows native behavior incl. the O_NOFOLLOW half of #8227 (declared follow-up); other dev:ino consumers read but not driven; ACP Session.ts producer verified by code read (same accepting end as the driven path); repo-wide suites not run.
Full report, harnesses, and raw logs: tmp/pr9857-verify-20260825-0545/ (report.md, verdict.txt, assertions.json, harnesses/, logs/, evidence/).
|
Released in v0.22.2. |
3 participants
What this PR does
Rejects validated
@-file reads when the approved or reopened file identity has an unverifiable inode value such as0, instead of treating{ dev, ino: 0 }as a usable identity match.Why it's needed
Issue #8227 points out that validated file reads rely on
(dev, ino)identity checks after approval. On filesystems where inode identity is unavailable or reported as0, matching on inode0is vacuous: multiple files can share the same placeholder identity, so the validation step cannot prove the reopened file is the one the user approved. The new guard fails closed for unverifiable inode identities while keeping the existing identity checks for normal filesystems.Reviewer Test Plan
How to verify
Run the targeted read-many-files tests, the core typecheck, and Prettier on the touched files.
Evidence (Before & After)
Before: a regression test with mocked
fs.stat()/ file-handlestat()returningino: 0would still include the validated file content. After: the same path is omitted from successful file content and returned as a visible read error explaining that the validated identity is unavailable on this filesystem.Tested on
Environment (optional)
Local Node/npm workspace on macOS.
Risk & Scope
Linked Issues
Refs #8227. This handles the inode-zero fail-closed behavior; Windows runner coverage and broader platform policy remain separate follow-up scope.
中文说明
What this PR does
当已批准或重新打开的文件身份里 inode 不可验证（例如
0）时，拒绝 validated@-file read，而不是把{ dev, ino: 0 }当作可用身份继续匹配。Why it's needed
#8227 指出 validated file read 在用户批准后依赖
(dev, ino)身份校验。在某些文件系统上，inode 身份不可用或会被报告为0；此时用 inode0匹配是空洞的，因为多个文件可能共享同一个占位身份，验证步骤无法证明重新打开的文件就是用户批准的文件。新逻辑对不可验证 inode fail closed，同时保留普通文件系统上的现有身份校验。Reviewer Test Plan
How to verify
运行目标 read-many-files 测试、core typecheck，并对改动文件跑 Prettier。
Evidence (Before & After)
Before：mock
fs.stat()/ file-handlestat()返回ino: 0的回归测试仍会包含 validated 文件内容。After：同一路径不会进入成功文件内容，而是作为可见读取错误返回，并说明当前文件系统无法提供 validated identity。Tested on
Environment (optional)
macOS 本地 Node/npm 工作区。
Risk & Scope
Linked Issues
Refs #8227. This handles the inode-zero fail-closed behavior; Windows runner coverage and broader platform policy remain separate follow-up scope.