mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-25 00:26:31 +00:00
fix(core): resolve validated attachment review findings
This commit is contained in:
parent
27b9182f11
commit
8f824fbbaf
5 changed files with 123 additions and 17 deletions
|
|
@ -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:<uri> as a resource ref when a server is named mcp', async () => {
|
||||
const readMcpResource = vi.fn().mockResolvedValue({
|
||||
contents: [{ uri: 'res://doc', text: 'RESOURCE BODY' }],
|
||||
|
|
|
|||
|
|
@ -221,6 +221,7 @@ export async function resolveAtCommandQuery({
|
|||
const atPathToResolvedSpecMap = new Map<string, string>();
|
||||
const contentLabelsForDisplay: string[] = [];
|
||||
const displayPaths = new Map<string, string>();
|
||||
const displayPathsByCanonicalPath = new Map<string, Set<string>>();
|
||||
const ignoredByReason: Record<string, string[]> = {
|
||||
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<string>();
|
||||
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 {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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<boolean>,
|
||||
): 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` };
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue