Skip to content

fix(core): reject unverifiable validated read inodes #9857

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
wenshao merged 5 commits into from
Aug 25, 2026
Merged

fix(core): reject unverifiable validated read inodes #9857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2072e1a
fix(core): reject unverifiable validated read inodes
Aug 24, 2026
33f2622
fix(core): surface unverifiable inode reads
Aug 24, 2026
407bcdc
fix(core): dedupe unverifiable inode read errors
Aug 24, 2026
6be6d7c
merge latest main into inode validated-read fix
Aug 24, 2026
4310865
test(core): share zero inode read mocks
Aug 24, 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
4 changes: 2 additions & 2 deletions docs/design/web-shell/web-shell-image-drag-and-drop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,8 +510,8 @@ BMP 以 `image/bmp` 进入缩略图 data URL 和 daemon image block。Core 的
> 及其 BMP 尺寸解析）已作为孤儿代码删除。BMP 支持现在仅依赖 `SUPPORTED_IMAGE_MIME_TYPES`
> 接受清单与 converter 透传；token 计数使用 `compactionInputSlimming.ts` 中的固定
> `DEFAULT_IMAGE_TOKEN_ESTIMATE`。下文对 BMP 路径的 E2E/人工验收要求不变。
浏览器若不能解码缩略图，不影响附件数据传输，但 E2E 必须覆盖 Chromium 解码，
Firefox/Linux 必须完成人工验收。
> 浏览器若不能解码缩略图，不影响附件数据传输，但 E2E 必须覆盖 Chromium 解码，
> Firefox/Linux 必须完成人工验收。

提交后的 user transcript 还经过 `isSafeImageSrc`，因此其被动位图 data-URI allowlist 必须
加入精确的 `image/bmp;base64,`，否则 composer 预览可见而 user message 会静默隐藏 BMP。
Expand Down
6 changes: 4 additions & 2 deletions packages/core/src/agents/runtime/agent-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,8 @@ export function extractParentToolNames(
new Set(
(
generationConfig?.tools as
Array<{ functionDeclarations?: FunctionDeclaration[] }> | undefined
| Array<{ functionDeclarations?: FunctionDeclaration[] }>
| undefined
)
?.flatMap((tool) => tool.functionDeclarations ?? [])
.map((declaration) => declaration.name)
Expand Down Expand Up @@ -1539,7 +1540,8 @@ export class AgentCore {
const registeredTool = this.runtimeContext
.getToolRegistry()
.getTool(toolName) as
{ serverName?: unknown; serverToolName?: unknown } | undefined;
| { serverName?: unknown; serverToolName?: unknown }
| undefined;
if (
typeof registeredTool?.serverName !== 'string' ||
typeof registeredTool.serverToolName !== 'string'
Expand Down
24 changes: 15 additions & 9 deletions packages/core/src/config/config-session-env.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,11 +486,13 @@ describe('Config.getModelRouteIdentity (#9454 route key)', () => {
baseUrl: 'https://route.example/v1',
} as ContentGeneratorConfig);
expect(explicit).toMatch(/^route-model@[0-9a-f]{8}$/);
expect(config.getModelRouteIdentity('route-model', {
model: 'route-model',
authType: 'openai',
baseUrl: 'https://route.example/v1',
} as ContentGeneratorConfig)).toBe(explicit);
expect(
config.getModelRouteIdentity('route-model', {
model: 'route-model',
authType: 'openai',
baseUrl: 'https://route.example/v1',
} as ContentGeneratorConfig),
).toBe(explicit);
});

it('does not mix the registry base URL into a non-active model identity', async () => {
Expand Down Expand Up @@ -523,13 +525,17 @@ describe('Config.getModelRouteIdentity (#9454 route key)', () => {

registrySpy.mockReturnValue('https://registry.example/v1');
const activeWithRegistry = config.getModelRouteIdentity();
const foreignWithRegistry =
config.getModelRouteIdentity('foreign-model', foreignGeneratorConfig);
const foreignWithRegistry = config.getModelRouteIdentity(
'foreign-model',
foreignGeneratorConfig,
);

registrySpy.mockReturnValue(null);
const activeWithoutRegistry = config.getModelRouteIdentity();
const foreignWithoutRegistry =
config.getModelRouteIdentity('foreign-model', foreignGeneratorConfig);
const foreignWithoutRegistry = config.getModelRouteIdentity(
'foreign-model',
foreignGeneratorConfig,
);

// The fallback is load-bearing for the ACTIVE model…
expect(activeWithRegistry).toMatch(/^active-model@[0-9a-f]{8}$/);
Expand Down
88 changes: 88 additions & 0 deletions packages/core/src/tools/readManyFiles.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,34 @@ describe('readManyFiles', () => {
return { relativePath, absolutePath };
}

function mockZeroInodeForPath(absolutePath: string): { restore(): void } {
const originalStat = fs.stat.bind(fs);
const originalOpen = fs.open.bind(fs);
const statSpy = vi.spyOn(fs, 'stat').mockImplementation(async (...args) => {
const stats = await originalStat(...args);
if (String(args[0]) === absolutePath) {
Object.defineProperty(stats, 'ino', { value: 0 });
}
return stats;
});
const openSpy = vi.spyOn(fs, 'open').mockImplementation(async (...args) => {
const handle = await originalOpen(...args);
const originalHandleStat = handle.stat.bind(handle);
vi.spyOn(handle, 'stat').mockImplementation(async () => {
const stats = await originalHandleStat();
Object.defineProperty(stats, 'ino', { value: 0 });
return stats;
});
return handle;
});
return {
restore: () => {
statSpy.mockRestore();
openSpy.mockRestore();
},
};
}

async function createTestDir(...pathSegments: string[]): Promise<string> {
const absolutePath = path.join(tempRootDir, ...pathSegments);
await fs.mkdir(absolutePath, { recursive: true });
Expand Down Expand Up @@ -184,6 +212,66 @@ describe('readManyFiles', () => {
expect(result.files).toHaveLength(0);
});

it('surfaces an error when validated inode identity is unverifiable', async () => {
const { relativePath, absolutePath } =
await createTestFile('zero-inode.txt');
const approvedStats = await fs.stat(absolutePath);
const zeroInodeMock = mockZeroInodeForPath(absolutePath);
const mockConfig = createMockConfig(tempRootDir);

try {
const result = await readManyFiles(mockConfig, {
paths: [relativePath],
validatedPathIdentities: new Map([
[absolutePath, { dev: approvedStats.dev, ino: 0 }],
]),
});

expect(contentToString(result.contentParts)).not.toContain(
'Content of zero-inode.txt',
);
expect(contentToString(result.contentParts)).toContain(
'Validated file identity is unavailable on this filesystem',
);
expect(result.files).toHaveLength(1);
expect(result.files[0]!.error).toContain(
'Validated file identity is unavailable on this filesystem',
);
} finally {
zeroInodeMock.restore();
}
});

it('deduplicates unverifiable validated inode errors for repeated paths', async () => {
const { relativePath, absolutePath } = await createTestFile(
'zero-inode-duplicate.txt',
Comment on lines +245 to +247

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 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] 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)

);
const approvedStats = await fs.stat(absolutePath);
const zeroInodeMock = mockZeroInodeForPath(absolutePath);
const mockConfig = createMockConfig(tempRootDir);

try {
const result = await readManyFiles(mockConfig, {
paths: [relativePath, relativePath],
validatedPathIdentities: new Map([
[absolutePath, { dev: approvedStats.dev, ino: 0 }],
]),
});
const content = contentToString(result.contentParts);

expect(content).not.toContain('Content of zero-inode-duplicate.txt');
expect(
content.match(/Validated file identity is unavailable/g),
).toHaveLength(1);
expect(result.files).toHaveLength(1);
expect(result.files[0]!.error).toContain(
'Validated file identity is unavailable',
);
} finally {
zeroInodeMock.restore();
}
});

it('drops a validated read when the identity map has no matching path key', async () => {
const { relativePath, absolutePath } =
await createTestFile('approved.txt');
Expand Down
77 changes: 54 additions & 23 deletions packages/core/src/tools/readManyFiles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
isCacheableReadResult,
processSingleFileContent,
} from '../utils/fileUtils.js';
import { hasVerifiableInode } from '../utils/file-identity.js';
import { getFolderStructure } from '../utils/getFolderStructure.js';

/**
Expand Down Expand Up @@ -159,6 +160,18 @@ export async function readManyFiles(
const displayPath = displayPaths?.get(fullPath) ?? fullPath;
const validatedIdentity = validatedPathIdentities?.get(fullPath);
if (validatedPathIdentities && !validatedIdentity) continue;
if (validatedIdentity && !hasVerifiableInode(validatedIdentity.ino)) {

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 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] 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)

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 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] 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)

if (!seenFiles.has(fullPath)) {
seenFiles.add(fullPath);
const { contentParts: errorParts, info } = createFileReadErrorResult(
displayPath,
'Validated file identity is unavailable on this filesystem (inode is 0).',
);
contentParts.push(...errorParts);
files.push(info);
}
continue;
}
if (
validatedIdentity &&
!(await matchesValidatedPathIdentity(fullPath, validatedIdentity))
Expand Down Expand Up @@ -223,18 +236,7 @@ export async function readManyFiles(
} catch (error) {
if (signal?.aborted || isAbortError(error)) throw error;
const errorMessage = getErrorMessage(error);
readResult = {
contentParts: [
{ text: `\nContent from ${displayPath}:\n` },
{ text: `Error reading ${displayPath}: ${errorMessage}` },
],
info: {
filePath: displayPath,
content: `Error reading ${displayPath}: ${errorMessage}`,
isDirectory: false,
error: errorMessage,
},
};
readResult = createFileReadErrorResult(displayPath, errorMessage);
}
} else {
try {
Expand Down Expand Up @@ -299,11 +301,7 @@ async function readValidatedTextFileContent(
);
try {
const stats = await source.stat();
if (
!stats.isFile() ||
stats.dev !== expected.dev ||
stats.ino !== expected.ino
) {
if (!fileStatsMatchValidatedIdentity(stats, expected)) {
return null;
}
return await readFileContent(
Expand Down Expand Up @@ -334,12 +332,30 @@ async function matchesValidatedPathIdentity(
const canonicalPath = await fs.promises.realpath(filePath);
if (canonicalPath !== filePath) return false;
const stats = await fs.promises.stat(canonicalPath);
return stats.dev === expected.dev && stats.ino === expected.ino;
return statsMatchValidatedIdentity(stats, expected);
} catch {
return false;
}
}

function statsMatchValidatedIdentity(
stats: fs.Stats,
expected: ReadManyFilesPathIdentity,
): boolean {
return (
hasVerifiableInode(stats.ino) &&
stats.dev === expected.dev &&
stats.ino === expected.ino
);
}

function fileStatsMatchValidatedIdentity(
stats: fs.Stats,
expected: ReadManyFilesPathIdentity,
): boolean {
return stats.isFile() && statsMatchValidatedIdentity(stats, expected);
}

async function snapshotValidatedFile(
filePath: string,
expected: ReadManyFilesPathIdentity,
Expand All @@ -364,11 +380,7 @@ async function snapshotValidatedFile(
);
try {
const stats = await source.stat();
if (
!stats.isFile() ||
stats.dev !== expected.dev ||
stats.ino !== expected.ino
) {
if (!fileStatsMatchValidatedIdentity(stats, expected)) {
return undefined;
}
if (stats.size > SNAPSHOT_MAX_SIZE_BYTES) {
Expand Down Expand Up @@ -468,6 +480,25 @@ async function readDirectory(
};
}

function createFileReadErrorResult(
displayPath: string,
errorMessage: string,
): { contentParts: Part[]; info: FileReadInfo } {
Comment on lines +483 to +486

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 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] 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)

const content = `Error reading ${displayPath}: ${errorMessage}`;
return {
contentParts: [
{ text: `\nContent from ${displayPath}:\n` },
{ text: content },
],
info: {
filePath: displayPath,
content,
isDirectory: false,
error: errorMessage,
},
};
}

async function readFileContent(
config: Config,
filePath: string,
Expand Down
Loading