From 8f824fbbaf7efb2b83deaf1d00f6dbd4e631e6e8 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Fri, 31 Jul 2026 03:23:35 +0800 Subject: [PATCH] fix(core): resolve validated attachment review findings --- .../src/ui/hooks/atCommandProcessor.test.ts | 44 +++++++++++++++++ .../cli/src/ui/hooks/atCommandProcessor.ts | 20 ++++++-- packages/core/src/utils/fileUtils.ts | 9 ++-- packages/core/src/utils/readManyFiles.test.ts | 47 +++++++++++++++++++ packages/core/src/utils/readManyFiles.ts | 20 +++++--- 5 files changed, 123 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts index 5968e7ed32..f948884450 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts @@ -1583,6 +1583,50 @@ describe('handleAtCommand', () => { }, ); + it.skipIf(process.platform === 'win32')( + 'prunes all labels for a skipped canonical file', + async () => { + const filePath = await createTestFile( + path.join(testRootDir, 'target.txt'), + 'safe content', + ); + const aliasPath = path.join(testRootDir, 'alias.txt'); + await fsPromises.symlink(filePath, aliasPath); + const outsideDir = await fsPromises.realpath( + await fsPromises.mkdtemp(path.join(os.tmpdir(), 'at-swap-outside-')), + ); + const outsidePath = path.join(outsideDir, 'secret.txt'); + await fsPromises.writeFile(outsidePath, 'outside secret'); + const readMcpResource = vi.fn(async () => { + await fsPromises.unlink(filePath); + await fsPromises.symlink(outsidePath, filePath); + return { + contents: [{ uri: 'res://doc', text: 'resource body' }], + }; + }); + + try { + const result = await handleAtCommand({ + query: `inspect @${aliasPath} @${filePath} @myserver:res://doc`, + config: makeResourceConfig(readMcpResource), + onDebugMessage: mockOnDebugMessage, + messageId: 6002, + signal: abortController.signal, + }); + + expect(JSON.stringify(result.processedQuery)).not.toContain( + 'outside secret', + ); + expect(result.filesRead).not.toContain(aliasPath); + expect(result.filesRead).not.toContain(filePath); + expect(result.filesRead).toContain('myserver:res://doc'); + } finally { + await fsPromises.unlink(filePath).catch(() => {}); + await fsPromises.rm(outsideDir, { recursive: true, force: true }); + } + }, + ); + it('preserves @mcp: as a resource ref when a server is named mcp', async () => { const readMcpResource = vi.fn().mockResolvedValue({ contents: [{ uri: 'res://doc', text: 'RESOURCE BODY' }], diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index 729c10a3b7..f77173f1c0 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -221,6 +221,7 @@ export async function resolveAtCommandQuery({ const atPathToResolvedSpecMap = new Map(); const contentLabelsForDisplay: string[] = []; const displayPaths = new Map(); + const displayPathsByCanonicalPath = new Map>(); const ignoredByReason: Record = { git: [], qwen: [], @@ -445,6 +446,10 @@ export async function resolveAtCommandQuery({ atPathToResolvedSpecMap.set(originalAtPath, pathName); contentLabelsForDisplay.push(pathName); displayPaths.set(canonicalPath, pathName); + const canonicalDisplays = + displayPathsByCanonicalPath.get(canonicalPath) ?? new Set(); + canonicalDisplays.add(pathName); + displayPathsByCanonicalPath.set(canonicalPath, canonicalDisplays); resolvedSuccessfully = true; } catch (error) { if (isNodeError(error) && error.code === 'ENOENT') { @@ -845,14 +850,21 @@ export async function resolveAtCommandQuery({ { dev: number; ino: number } >(); const pruneSkippedPath = (approvedPath: string) => { - const displayPath = displayPaths.get(approvedPath) ?? approvedPath; + const displayPath = displayPaths.get(approvedPath); + const displayLabels = + displayPathsByCanonicalPath.get(approvedPath) ?? + new Set(displayPath ? [displayPath] : []); for (const [originalAtPath, resolvedSpec] of atPathToResolvedSpecMap) { - if (resolvedSpec === displayPath) { + if (resolvedSpec === approvedPath || displayLabels.has(resolvedSpec)) { atPathToResolvedSpecMap.delete(originalAtPath); } } - const index = contentLabelsForDisplay.indexOf(displayPath); - if (index >= 0) contentLabelsForDisplay.splice(index, 1); + for (let index = contentLabelsForDisplay.length - 1; index >= 0; index--) { + const label = contentLabelsForDisplay[index]; + if (label === approvedPath || displayLabels.has(label)) { + contentLabelsForDisplay.splice(index, 1); + } + } }; for (const approvedPath of pathSpecsToRead) { try { diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 9b2e59e0c4..c3e2e495d5 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -1877,13 +1877,10 @@ export async function processSingleFileContent( throw error; } const errorMessage = getErrorMessage(error); - const relativeDisplayPath = path - .relative(rootDirectory, displayPath) - .replace(/\\/g, '/'); return { - llmContent: `Error reading file ${relativeDisplayPath}: ${errorMessage}`, - returnDisplay: `Error reading file ${relativeDisplayPath}: ${errorMessage}`, - error: `Error reading file ${relativeDisplayPath}: ${errorMessage}`, + llmContent: `Error reading file ${relativePathForDisplay}: ${errorMessage}`, + returnDisplay: `Error reading file ${relativePathForDisplay}: ${errorMessage}`, + error: `Error reading file ${relativePathForDisplay}: ${errorMessage}`, errorType: ToolErrorType.READ_CONTENT_FAILURE, }; } diff --git a/packages/core/src/utils/readManyFiles.test.ts b/packages/core/src/utils/readManyFiles.test.ts index 95ea243005..ea3e8499ff 100644 --- a/packages/core/src/utils/readManyFiles.test.ts +++ b/packages/core/src/utils/readManyFiles.test.ts @@ -285,6 +285,53 @@ describe('readManyFiles', () => { expect(contentToString(result.contentParts)).toContain('unsaved buffer'); }); + it('does not cache a validated custom-fs read dropped after identity drift', async () => { + const { relativePath, absolutePath } = + await createTestFile('approved.txt'); + const backupPath = `${absolutePath}.approved`; + const stats = await fs.stat(absolutePath); + const cache = new FileReadCache(); + const readTextFile = vi.fn(async () => { + await fs.rename(absolutePath, backupPath); + await fs.writeFile(absolutePath, 'replacement secret'); + return { + content: 'unsaved buffer', + _meta: { + originalLineCount: 1, + originalLineCountExact: true, + }, + }; + }); + const mockConfig = { + ...createMockConfigWithCache(tempRootDir, cache), + getFileSystemService: () => ({ + readTextFile, + writeTextFile: vi.fn(), + findFiles: vi.fn(), + }), + } as unknown as Config; + + try { + const result = await readManyFiles(mockConfig, { + paths: [relativePath], + validatedPathIdentities: new Map([ + [absolutePath, { dev: stats.dev, ino: stats.ino }], + ]), + }); + + const content = contentToString(result.contentParts); + expect(content).not.toContain('unsaved buffer'); + expect(content).not.toContain('replacement secret'); + expect(result.files).toHaveLength(0); + expect(cache.size()).toBe(0); + const decision = await checkPriorRead(cache, absolutePath, 'editing'); + expect(decision.ok).toBe(false); + } finally { + await fs.unlink(absolutePath).catch(() => {}); + await fs.rename(backupPath, absolutePath).catch(() => {}); + } + }); + it('should include truncated large text files instead of reporting a size error', async () => { const relativePath = 'large.log'; const absolutePath = path.join(tempRootDir, relativePath); diff --git a/packages/core/src/utils/readManyFiles.ts b/packages/core/src/utils/readManyFiles.ts index a30e3c4f9b..1ae868a55e 100644 --- a/packages/core/src/utils/readManyFiles.ts +++ b/packages/core/src/utils/readManyFiles.ts @@ -186,6 +186,10 @@ export async function readManyFiles( if (shouldSnapshot && !snapshot) continue; let readResult; try { + const validateAfterRead = + validatedIdentity && !snapshot + ? () => matchesValidatedPathIdentity(fullPath, validatedIdentity) + : undefined; readResult = await readFileContent( config, snapshot?.filePath ?? fullPath, @@ -193,17 +197,11 @@ export async function readManyFiles( signal, displayPath, snapshot?.stats, + validateAfterRead, ); } finally { await snapshot?.cleanup(); } - if ( - validatedIdentity && - !snapshot && - !(await matchesValidatedPathIdentity(fullPath, validatedIdentity)) - ) { - continue; - } if (readResult) { contentParts.push(...readResult.contentParts); files.push(readResult.info); @@ -341,6 +339,10 @@ async function snapshotValidatedFile( } } catch (error) { if (signal?.aborted || isAbortError(error)) throw error; + if (result) { + await result.cleanup(); + result = undefined; + } return undefined; } finally { if (snapshotDir && !result) { @@ -384,6 +386,7 @@ async function readFileContent( signal?: AbortSignal, displayPath = filePath, validatedStats?: fs.Stats, + validateAfterRead?: () => Promise, ): Promise<{ contentParts: Part[]; info: FileReadInfo } | null> { try { const fileReadResult = await processSingleFileContent(filePath, config, { @@ -395,6 +398,9 @@ async function readFileContent( if (validatedStats && fileReadResult.stats) { fileReadResult.stats = validatedStats; } + if (validateAfterRead && !(await validateAfterRead())) { + return null; + } const prefixText: Part = { text: `\nContent from ${displayPath}:\n` };