From 27e7f964670f616bfe43a12cc2cf46378ab3d644 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 12:20:25 -0700 Subject: [PATCH 1/5] fix: make script template creation reliable Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/managing-python-projects.md | 4 +- .../script-copilot-instructions.md | 4 +- .../templates/new723ScriptTemplate/script.py | 7 +- src/features/creators/newScriptProject.ts | 310 ++++++-- .../creators/newScriptProject.unit.test.ts | 690 +++++++++++++++++- 5 files changed, 930 insertions(+), 85 deletions(-) diff --git a/docs/managing-python-projects.md b/docs/managing-python-projects.md index 9f64f773d..21bfcd971 100644 --- a/docs/managing-python-projects.md +++ b/docs/managing-python-projects.md @@ -75,9 +75,9 @@ Use this to scaffold a brand new Python project with the correct structure and f - **Package**: A structured Python package with `pyproject.toml`, tests folder, and package directory - **Script**: A simple standalone Python file using PEP 723 inline metadata 4. Enter a name for your project. -5. Choose whether to create a virtual environment. +5. If you're creating a package, choose whether to create a virtual environment. -The extension creates the project structure, adds it to your workspace, and optionally creates a virtual environment. +The extension creates the project structure and adds it to your workspace. For packages, it can also create a virtual environment. #### Package template structure diff --git a/files/templates/copilot-instructions-text/script-copilot-instructions.md b/files/templates/copilot-instructions-text/script-copilot-instructions.md index 01bd826a6..130afb400 100644 --- a/files/templates/copilot-instructions-text/script-copilot-instructions.md +++ b/files/templates/copilot-instructions-text/script-copilot-instructions.md @@ -1,7 +1,7 @@ # Copilot Instructions for -- The script `` is a Python python project within the workspace. -- It has inline script metadata (as proposed by PEP 723) that defines the script name, required python version, and dependencies. +- The script `` is a Python project within the workspace. +- It has inline script metadata (as proposed by PEP 723) that defines the required Python version and dependencies. - If imports which require a specific Python version or dependencies are added, keep the inline script metadata up to date. - You need to call the `Get Python Environment Information` tool on the `` path to get the Python executable details. - Substitute the Python executable you get from the `Get Python Environment Information` tool anywhere you see `` in these instructions. diff --git a/files/templates/new723ScriptTemplate/script.py b/files/templates/new723ScriptTemplate/script.py index 7511eea1f..74abfb6e3 100644 --- a/files/templates/new723ScriptTemplate/script.py +++ b/files/templates/new723ScriptTemplate/script.py @@ -1,8 +1,7 @@ # /// script -# requires-python = ">=X.XX" TODO: Update this to the minimum Python version you want to support -# dependencies = [ -# TODO: Add any dependencies your script requires -# ] +# requires-python = ">=3.9" +# dependencies = [] +# # Add dependency requirement strings to the array above as needed. # /// # TODO: Update the main function to your needs or remove it. diff --git a/src/features/creators/newScriptProject.ts b/src/features/creators/newScriptProject.ts index 5a62b6cb4..3056aadfb 100644 --- a/src/features/creators/newScriptProject.ts +++ b/src/features/creators/newScriptProject.ts @@ -1,26 +1,117 @@ import * as fs from 'fs-extra'; import * as path from 'path'; -import { commands, l10n, MarkdownString, QuickInputButtons, Uri, window, workspace } from 'vscode'; +import { commands, l10n, MarkdownString, QuickInputButtons, Uri, window, WorkspaceFolder } from 'vscode'; import { PythonProject, PythonProjectCreator, PythonProjectCreatorOptions } from '../../api'; import { NEW_PROJECT_TEMPLATES_FOLDER } from '../../common/constants'; import { traceError } from '../../common/logging'; -import { showInputBoxWithButtons, showTextDocument } from '../../common/window.apis'; +import { normalizePath } from '../../common/utils/pathUtils'; +import { isWindows } from '../../common/utils/platformUtils'; +import { showErrorMessage, showInputBoxWithButtons, showTextDocument } from '../../common/window.apis'; +import { getWorkspaceFolder, getWorkspaceFolders } from '../../common/workspace.apis'; import { PythonProjectManager } from '../../internal.api'; import { isCopilotInstalled, manageCopilotInstructionsFile, replaceInFilesAndNames } from './creationHelpers'; +function validateScriptFileName(value: string): string | null { + const pathSegments = value.split(/[\\/]/); + if (pathSegments.length !== 1 || pathSegments.includes('..')) { + return l10n.t('Script name must be a file name without path separators or traversal.'); + } + if (!value.endsWith('.py')) { + return l10n.t('Script name must end with ".py".'); + } + const baseName = value.replace(/\.py$/, ''); + const deviceBaseName = baseName.split('.')[0]; + if (isWindows() && /^(con|prn|aux|nul|com[1-9]|lpt[1-9])$/i.test(deviceBaseName)) { + return l10n.t('Script name uses a reserved Windows device name.'); + } + // following PyPI (PEP 508) rules for package names + if (!/^([a-z_]|[a-z0-9_][a-z0-9._-]*[a-z0-9_])$/i.test(baseName)) { + return l10n.t( + 'Invalid script name. Use only letters, numbers, underscores, hyphens, or periods. Must start and end with a letter or number.', + ); + } + if (/^[-._0-9]$/i.test(baseName)) { + return l10n.t('Single-character script names cannot be a number, hyphen, or period.'); + } + return null; +} + +function isSameOrDescendantPath(parentPath: string, candidatePath: string): boolean { + const relativePath = path.relative( + normalizePath(path.resolve(parentPath)), + normalizePath(path.resolve(candidatePath)), + ); + return ( + relativePath === '' || + (relativePath !== '..' && !relativePath.startsWith(`..${path.sep}`) && !path.isAbsolute(relativePath)) + ); +} + +function uriForFileRootInWorkspace(rootPath: string, workspaceFolder: WorkspaceFolder): Uri { + const relativeRootPath = path.relative(path.resolve(workspaceFolder.uri.fsPath), path.resolve(rootPath)); + const pathSegments = relativeRootPath.split(/[\\/]/).filter((segment) => segment.length > 0); + return workspaceFolder.uri.with({ + path: path.posix.join(workspaceFolder.uri.path, ...pathSegments), + }); +} + +async function lstatIfExists(candidatePath: string) { + try { + return await fs.lstat(candidatePath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return undefined; + } + throw error; + } +} + +async function isCopilotInstructionsDestinationContained( + destinationRoot: string, + physicalDestinationRoot: string, + physicalWorkspaceRoot: string, +): Promise { + try { + const githubFolder = path.join(destinationRoot, '.github'); + const githubEntry = await lstatIfExists(githubFolder); + if (!githubEntry) { + return isSameOrDescendantPath(physicalWorkspaceRoot, physicalDestinationRoot); + } + + const physicalGithubFolder = await fs.realpath(githubFolder); + if (!isSameOrDescendantPath(physicalWorkspaceRoot, physicalGithubFolder)) { + return false; + } + if (!(await fs.stat(githubFolder)).isDirectory()) { + return false; + } + + const instructionsFile = path.join(githubFolder, 'copilot-instructions.md'); + const instructionsEntry = await lstatIfExists(instructionsFile); + if (!instructionsEntry) { + return true; + } + if (instructionsEntry.isSymbolicLink() || !instructionsEntry.isFile()) { + return false; + } + + const physicalInstructionsFile = await fs.realpath(instructionsFile); + return isSameOrDescendantPath(physicalWorkspaceRoot, physicalInstructionsFile); + } catch (error) { + traceError('Failed to validate the Copilot instructions destination:', error); + return false; + } +} + export class NewScriptProject implements PythonProjectCreator { public readonly name = l10n.t('newScript'); public readonly displayName = l10n.t('Script'); - public readonly description = l10n.t('Creates a new script folder in your current workspace'); + public readonly description = l10n.t('Creates a new script in your current workspace'); public readonly tooltip = new MarkdownString(l10n.t('Create a new Python script')); constructor(private readonly projectManager: PythonProjectManager) {} async create(options?: PythonProjectCreatorOptions): Promise { - // quick create (needs name, will always create venv and copilot instructions) - // not quick create - // ask for script file name - // ask if they want venv let scriptFileName = options?.name; let createCopilotInstructions: boolean | undefined; if (options?.quickCreate === true) { @@ -28,6 +119,9 @@ export class NewScriptProject implements PythonProjectCreator { if (!scriptFileName) { throw new Error('Script file name is required in quickCreate mode.'); } + if (path.extname(scriptFileName) === '') { + scriptFileName = `${scriptFileName}.py`; + } createCopilotInstructions = true; } else { //Prompt as quickCreate is false @@ -37,23 +131,7 @@ export class NewScriptProject implements PythonProjectCreator { prompt: l10n.t('What is the name of the script? (e.g. my_script.py)'), ignoreFocusOut: true, showBackButton: true, - validateInput: (value) => { - // Ensure the filename ends with .py and follows valid naming conventions - if (!value.endsWith('.py')) { - return l10n.t('Script name must end with ".py".'); - } - const baseName = value.replace(/\.py$/, ''); - // following PyPI (PEP 508) rules for package names - if (!/^([a-z_]|[a-z0-9_][a-z0-9._-]*[a-z0-9_])$/i.test(baseName)) { - return l10n.t( - 'Invalid script name. Use only letters, numbers, underscores, hyphens, or periods. Must start and end with a letter or number.', - ); - } - if (/^[-._0-9]$/i.test(baseName)) { - return l10n.t('Single-character script names cannot be a number, hyphen, or period.'); - } - return null; - }, + validateInput: validateScriptFileName, }); } catch (ex) { if (ex === QuickInputButtons.Back) { @@ -67,64 +145,152 @@ export class NewScriptProject implements PythonProjectCreator { createCopilotInstructions = true; } } + } + const validationError = validateScriptFileName(scriptFileName); + if (validationError) { + if (options?.quickCreate === true) { + throw new Error(validationError); + } + window.showErrorMessage(validationError); + return undefined; + } - // 1. Copy template folder - const newScriptTemplateFile = path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'new723ScriptTemplate', 'script.py'); - if (!(await fs.pathExists(newScriptTemplateFile))) { - window.showErrorMessage(l10n.t('Template file does not exist, aborting creation.')); - traceError(`Template file not found at: ${newScriptTemplateFile}`); + // 1. Copy template file + const newScriptTemplateFile = path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'new723ScriptTemplate', 'script.py'); + if (!(await fs.pathExists(newScriptTemplateFile))) { + window.showErrorMessage(l10n.t('Template file does not exist, aborting creation.')); + traceError(`Template file not found at: ${newScriptTemplateFile}`); + return undefined; + } + + // Check if the destination folder is provided, otherwise use the first workspace folder. + let destinationRootUri = options?.rootUri; + let workspaceFolders: readonly WorkspaceFolder[] | undefined; + if (!destinationRootUri) { + workspaceFolders = getWorkspaceFolders(); + if (!workspaceFolders || workspaceFolders.length === 0) { + window.showErrorMessage(l10n.t('No workspace folder is open or provided, aborting creation.')); return undefined; } + destinationRootUri = workspaceFolders[0].uri; + } - // Check if the destination folder is provided, otherwise use the first workspace folder - let destRoot = options?.rootUri.fsPath; - if (!destRoot) { - const workspaceFolders = workspace.workspaceFolders; - if (!workspaceFolders || workspaceFolders.length === 0) { - window.showErrorMessage(l10n.t('No workspace folder is open or provided, aborting creation.')); - return undefined; - } - destRoot = workspaceFolders[0].uri.fsPath; + const destRoot = destinationRootUri.fsPath; + const resolvedDestRoot = path.resolve(destRoot); + let workspaceFolder = getWorkspaceFolder(destinationRootUri); + if (!workspaceFolder && destinationRootUri.scheme === 'file') { + workspaceFolders ??= getWorkspaceFolders(); + workspaceFolder = workspaceFolders + ?.filter((folder) => isSameOrDescendantPath(folder.uri.fsPath, resolvedDestRoot)) + .sort((first, second) => second.uri.fsPath.length - first.uri.fsPath.length)[0]; + } + if (!workspaceFolder) { + showErrorMessage(l10n.t('Destination folder must be inside an open workspace, aborting creation.')); + return undefined; + } + + let physicalDestRoot: string; + let physicalWorkspaceRoot: string; + try { + [physicalDestRoot, physicalWorkspaceRoot] = await Promise.all([ + fs.realpath(resolvedDestRoot), + fs.realpath(workspaceFolder.uri.fsPath), + ]); + } catch (error) { + traceError('Failed to resolve the destination or workspace folder:', error); + showErrorMessage(l10n.t('Unable to resolve the destination folder inside the open workspace.')); + return undefined; + } + if (!isSameOrDescendantPath(physicalWorkspaceRoot, physicalDestRoot)) { + showErrorMessage(l10n.t('Destination folder must resolve inside the open workspace, aborting creation.')); + return undefined; + } + + const identityRootUri = + destinationRootUri.scheme === 'file' && workspaceFolder.uri.scheme !== 'file' + ? uriForFileRootInWorkspace(resolvedDestRoot, workspaceFolder) + : destinationRootUri; + const scriptDestination = path.resolve(resolvedDestRoot, scriptFileName); + const relativeScriptPath = path.relative(resolvedDestRoot, scriptDestination); + if ( + relativeScriptPath === '' || + relativeScriptPath === '..' || + relativeScriptPath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativeScriptPath) + ) { + const containmentError = l10n.t('Script name must resolve to a file inside the destination folder.'); + if (options?.quickCreate === true) { + throw new Error(containmentError); } + window.showErrorMessage(containmentError); + return undefined; + } - // Check if the destination folder already exists - const scriptDestination = path.join(destRoot, scriptFileName); - if (await fs.pathExists(scriptDestination)) { - window.showErrorMessage( - l10n.t( - 'A script file by that name already exists, aborting creation. Please retry with a unique script name given your workspace.', - ), - ); - return undefined; + if ( + createCopilotInstructions && + !(await isCopilotInstructionsDestinationContained( + resolvedDestRoot, + physicalDestRoot, + physicalWorkspaceRoot, + )) + ) { + showErrorMessage( + l10n.t('Copilot instructions must be stored inside the open workspace, aborting creation.'), + ); + return undefined; + } + + // Check if the destination file already exists + if (await fs.pathExists(scriptDestination)) { + window.showErrorMessage( + l10n.t( + 'A script file by that name already exists, aborting creation. Please retry with a unique script name given your workspace.', + ), + ); + return undefined; + } + await fs.copy(newScriptTemplateFile, scriptDestination); + + // 2. Replace 'script_name' in the file using a helper (just script name remove .py) + await replaceInFilesAndNames(scriptDestination, 'script_name', scriptFileName.replace(/\.py$/, '')); + + // Add the created script to the project manager + const createdScript: PythonProject = { + name: scriptFileName, + uri: identityRootUri.with({ + path: path.posix.join(identityRootUri.path, scriptFileName), + }), + }; + try { + await this.projectManager.add(createdScript); + } catch (registrationError) { + try { + this.projectManager.remove(createdScript); + } catch (rollbackError) { + traceError('Failed to remove the new script project after registration failed:', rollbackError); } - await fs.copy(newScriptTemplateFile, scriptDestination); - - // 2. Replace 'script_name' in the file using a helper (just script name remove .py) - await replaceInFilesAndNames(scriptDestination, 'script_name', scriptFileName.replace(/\.py$/, '')); - - // 3. add custom github copilot instructions - if (createCopilotInstructions) { - const packageInstructionsPath = path.join( - NEW_PROJECT_TEMPLATES_FOLDER, - 'copilot-instructions-text', - 'script-copilot-instructions.md', - ); - await manageCopilotInstructionsFile(destRoot, packageInstructionsPath, [ - { searchValue: '', replaceValue: scriptFileName }, - ]); + try { + await fs.remove(scriptDestination); + } catch (rollbackError) { + traceError('Failed to delete the new script after registration failed:', rollbackError); } + throw registrationError; + } - // Add the created script to the project manager - const createdScript: PythonProject | undefined = { - name: scriptFileName, - uri: Uri.file(scriptDestination), - }; - this.projectManager.add(createdScript); + // 3. add custom github copilot instructions + if (createCopilotInstructions) { + const packageInstructionsPath = path.join( + NEW_PROJECT_TEMPLATES_FOLDER, + 'copilot-instructions-text', + 'script-copilot-instructions.md', + ); + await manageCopilotInstructionsFile(destRoot, packageInstructionsPath, [ + { searchValue: '', replaceValue: scriptFileName }, + ]); + } - await showTextDocument(createdScript.uri); + await showTextDocument(createdScript.uri); - return createdScript; - } - return undefined; + return createdScript; } } diff --git a/src/test/features/creators/newScriptProject.unit.test.ts b/src/test/features/creators/newScriptProject.unit.test.ts index 897544f35..128626e2b 100644 --- a/src/test/features/creators/newScriptProject.unit.test.ts +++ b/src/test/features/creators/newScriptProject.unit.test.ts @@ -1,7 +1,19 @@ import assert from 'assert'; +import fsExtra from 'fs-extra'; import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; +import * as sinon from 'sinon'; +import { TextEditor, Uri, WorkspaceFolder } from 'vscode'; +import { PythonProject } from '../../../api'; +import { NEW_PROJECT_TEMPLATES_FOLDER } from '../../../common/constants'; +import { readInlineScriptMetadata } from '../../../common/inlineScript/metadata'; +import * as platformUtils from '../../../common/utils/platformUtils'; +import * as windowApis from '../../../common/window.apis'; +import * as workspaceApis from '../../../common/workspace.apis'; +import * as creationHelpers from '../../../features/creators/creationHelpers'; +import { NewScriptProject } from '../../../features/creators/newScriptProject'; +import { PythonProjectManager } from '../../../internal.api'; // Path to the real script template, resolved from the compiled test location // (out/test/features/creators/ → workspaceRoot/files/templates/...). We do NOT @@ -20,25 +32,693 @@ const TEMPLATE_PATH = path.join( 'script.py', ); +function asRemoteUri(fsPath: string, authority = 'ssh-remote+test-host'): Uri { + return Uri.from({ + scheme: 'vscode-remote', + authority, + path: Uri.file(fsPath).path, + }); +} + suite('new723ScriptTemplate / NewScriptProject', () => { let tmpDir: string; + let getWorkspaceFolderStub: sinon.SinonStub; + let getWorkspaceFoldersStub: sinon.SinonStub; + let workspaceFolder: WorkspaceFolder; setup(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'new-script-test-')); + workspaceFolder = { + index: 0, + name: 'test-workspace', + uri: Uri.file(tmpDir), + }; + getWorkspaceFolderStub = sinon.stub(workspaceApis, 'getWorkspaceFolder').returns(workspaceFolder); + getWorkspaceFoldersStub = sinon.stub(workspaceApis, 'getWorkspaceFolders').returns([workspaceFolder]); }); teardown(async () => { + sinon.restore(); await fs.remove(tmpDir); }); - test('Template file starts with a valid PEP 723 header (# /// script)', async () => { + function stubSuccessfulFileCreation() { + const showTextDocumentStub = sinon + .stub(windowApis, 'showTextDocument') + .resolves({} as TextEditor); + const pathExistsStub = sinon.stub(fsExtra, 'pathExists'); + pathExistsStub.onFirstCall().resolves(true); + pathExistsStub.onSecondCall().resolves(false); + const copyStub = sinon.stub(fsExtra, 'copy').callsFake(async (_source, destination) => { + await fs.copyFile(TEMPLATE_PATH, destination); + }); + const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); + const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); + return { copyStub, instructionsStub, replaceStub, showTextDocumentStub }; + } + + async function createDirectoryLink(target: string, link: string): Promise { + try { + await fs.symlink(target, link, platformUtils.isWindows() ? 'junction' : 'dir'); + return true; + } catch { + await fs.ensureDir(link); + return false; + } + } + + async function createFileLink(target: string, link: string): Promise { + try { + await fs.symlink(target, link, 'file'); + return true; + } catch { + await fs.copyFile(target, link); + return false; + } + } + + test('shipped template contains parseable PEP 723 metadata', async () => { const contents = await fs.readFile(TEMPLATE_PATH, 'utf8'); - const firstNonBlankLine = contents.split(/\r?\n/).find((l) => l.trim().length > 0); + const metadata = readInlineScriptMetadata(contents); + + assert.ok(metadata, 'Template must contain valid PEP 723 metadata'); + assert.strictEqual(metadata.range.start, 0, 'PEP 723 metadata should be at the start of the template'); + assert.strictEqual(metadata.requiresPython, '>=3.9'); + assert.notStrictEqual(metadata.dependencies, undefined, 'Template should declare dependencies'); + assert.deepStrictEqual(metadata.dependencies, []); + assert.strictEqual(metadata.tool, undefined); + }); + + test('interactive filename validation rejects unsafe names and accepts a valid name', async () => { + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons').callsFake(async (options) => { + const validateInput = options?.validateInput; + assert.ok(validateInput); + assert.strictEqual(typeof (await validateInput('../escape.py')), 'string'); + assert.strictEqual(typeof (await validateInput('nested/script.py')), 'string'); + assert.strictEqual(typeof (await validateInput('script')), 'string'); + assert.strictEqual(typeof (await validateInput('invalid name.py')), 'string'); + assert.strictEqual(await validateInput('valid_script.py'), null); + return undefined; + }); + + const result = await creator.create(); + + assert.strictEqual(result, undefined); + assert.ok(promptStub.calledOnce); + assert.strictEqual(addStub.called, false); + }); + + test('Windows filename validation rejects reserved device names and accepts similar names', async () => { + sinon.stub(platformUtils, 'isWindows').returns(true); + const creator = new NewScriptProject({ add: sinon.stub().resolves() } as unknown as PythonProjectManager); + sinon.stub(windowApis, 'showInputBoxWithButtons').callsFake(async (options) => { + const validateInput = options?.validateInput; + assert.ok(validateInput); + const reservedNames = [ + 'CON.py', + 'prn.py', + 'Aux.data.py', + 'nul.foo.py', + ...Array.from({ length: 9 }, (_, index) => `cOm${index + 1}.py`), + ...Array.from({ length: 9 }, (_, index) => `LpT${index + 1}.extra.py`), + ]; + for (const reservedName of reservedNames) { + assert.strictEqual( + typeof (await validateInput(reservedName)), + 'string', + `${reservedName} should be rejected on Windows`, + ); + } + for (const validName of ['console.py', 'com10.py', 'lpt10.py']) { + assert.strictEqual( + await validateInput(validName), + null, + `${validName} should remain valid on Windows`, + ); + } + return undefined; + }); + + assert.strictEqual(await creator.create(), undefined); + await assert.rejects( + creator.create({ + name: 'cOn', + quickCreate: true, + rootUri: Uri.file(tmpDir), + }), + /reserved Windows device name/, + ); + }); + + test('non-Windows filename validation allows Windows device names', async () => { + sinon.stub(platformUtils, 'isWindows').returns(false); + const creator = new NewScriptProject({ add: sinon.stub().resolves() } as unknown as PythonProjectManager); + sinon.stub(windowApis, 'showInputBoxWithButtons').callsFake(async (options) => { + const validateInput = options?.validateInput; + assert.ok(validateInput); + assert.strictEqual(await validateInput('CON.py'), null); + assert.strictEqual(await validateInput('con.foo.py'), null); + return undefined; + }); + + assert.strictEqual(await creator.create(), undefined); + }); + + test('quick create rejects a traversal name without prompting', async () => { + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons'); + const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); + + await assert.rejects( + creator.create({ + name: '../escape.py', + quickCreate: true, + rootUri: Uri.file(tmpDir), + }), + /path separators or traversal/, + ); + assert.strictEqual(promptStub.called, false); + assert.strictEqual(addStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + }); + + test('quick create appends .py to a safe extensionless base name', async () => { + const rootUri = Uri.file(tmpDir); + const scriptDestination = path.resolve(rootUri.fsPath, 'hello_world.py'); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons'); + const { showTextDocumentStub } = stubSuccessfulFileCreation(); + + const result = await creator.create({ + name: 'hello_world', + quickCreate: true, + rootUri, + }); + + assert.ok(result); + const createdScript = result as PythonProject; + assert.strictEqual(createdScript.name, 'hello_world.py'); + assert.strictEqual(createdScript.uri.fsPath, Uri.file(scriptDestination).fsPath); + assert.strictEqual(promptStub.called, false); + assert.ok(addStub.calledOnceWithExactly(createdScript)); + assert.ok(showTextDocumentStub.calledOnceWithExactly(createdScript.uri)); + }); + + test('quick create rejects a non-Python extension without prompting', async () => { + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons'); + const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); + + await assert.rejects( + creator.create({ + name: 'hello_world.txt', + quickCreate: true, + rootUri: Uri.file(tmpDir), + }), + /must end with ".py"/, + ); + + assert.strictEqual(promptStub.called, false); + assert.strictEqual(addStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + }); + + test('quick create accepts a valid name inside an open workspace without prompting', async () => { + const scriptFileName = 'quick_script.py'; + const rootUri = Uri.file(tmpDir); + const scriptDestination = path.resolve(rootUri.fsPath, scriptFileName); + const expectedTemplatePath = path.join( + NEW_PROJECT_TEMPLATES_FOLDER, + 'new723ScriptTemplate', + 'script.py', + ); + const addStub = sinon.stub().resolves(); + const projectManager = { add: addStub } as unknown as PythonProjectManager; + const creator = new NewScriptProject(projectManager); + + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons'); + const { copyStub, instructionsStub, replaceStub, showTextDocumentStub } = stubSuccessfulFileCreation(); + + const result = await creator.create({ + name: scriptFileName, + quickCreate: true, + rootUri, + }); + + assert.ok(result); + const createdScript = result as PythonProject; + assert.strictEqual(createdScript.name, scriptFileName); + assert.strictEqual(createdScript.uri.fsPath, Uri.file(scriptDestination).fsPath); + assert.strictEqual(await fs.readFile(scriptDestination, 'utf8'), await fs.readFile(TEMPLATE_PATH, 'utf8')); + assert.strictEqual(promptStub.called, false, 'quick create must not prompt for a script name'); + assert.ok(getWorkspaceFolderStub.calledOnce); assert.strictEqual( - firstNonBlankLine, - '# /// script', - 'Template must start with a valid PEP 723 `script` header line', + getWorkspaceFolderStub.firstCall.args[0].fsPath, + Uri.file(path.resolve(rootUri.fsPath)).fsPath, + ); + assert.ok(copyStub.calledOnce); + assert.strictEqual(copyStub.firstCall.args[0], expectedTemplatePath); + assert.strictEqual(copyStub.firstCall.args[1], scriptDestination); + assert.ok( + replaceStub.calledOnceWithExactly(scriptDestination, 'script_name', 'quick_script'), + 'template substitution should run', + ); + assert.ok(addStub.calledOnceWithExactly(createdScript), 'created script should be added as a project'); + assert.ok( + showTextDocumentStub.calledOnceWithExactly(createdScript.uri), + 'created script should be opened in the editor', + ); + assert.ok( + instructionsStub.calledOnceWithExactly( + rootUri.fsPath, + path.join( + NEW_PROJECT_TEMPLATES_FOLDER, + 'copilot-instructions-text', + 'script-copilot-instructions.md', + ), + [{ searchValue: '', replaceValue: scriptFileName }], + ), + 'quick create should retain Copilot-instruction handling', + ); + }); + + test('interactive fallback preserves a remote workspace URI', async () => { + const remoteWorkspaceUri = asRemoteUri(tmpDir); + workspaceFolder = { + index: 0, + name: 'remote-workspace', + uri: remoteWorkspaceUri, + }; + getWorkspaceFoldersStub.returns([workspaceFolder]); + getWorkspaceFolderStub.callsFake((uri: Uri) => + uri.toString() === remoteWorkspaceUri.toString() ? workspaceFolder : undefined, ); + sinon.stub(windowApis, 'showInputBoxWithButtons').resolves('remote_script.py'); + const { showTextDocumentStub } = stubSuccessfulFileCreation(); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create(); + + assert.ok(result); + const createdScript = result as PythonProject; + assert.strictEqual(getWorkspaceFolderStub.firstCall.args[0].toString(), remoteWorkspaceUri.toString()); + assert.strictEqual(createdScript.uri.scheme, remoteWorkspaceUri.scheme); + assert.strictEqual(createdScript.uri.authority, remoteWorkspaceUri.authority); + assert.strictEqual(createdScript.uri.fsPath, Uri.file(path.join(tmpDir, 'remote_script.py')).fsPath); + assert.ok(addStub.calledOnceWithExactly(createdScript)); + assert.ok(showTextDocumentStub.calledOnceWithExactly(createdScript.uri)); + }); + + test('quick file root matches by fsPath and retains the remote workspace URI identity', async () => { + const nestedRoot = path.join(tmpDir, 'nested'); + await fs.ensureDir(nestedRoot); + const remoteWorkspaceUri = asRemoteUri(tmpDir, 'dev-container+test'); + workspaceFolder = { + index: 0, + name: 'remote-workspace', + uri: remoteWorkspaceUri, + }; + getWorkspaceFolderStub.returns(undefined); + getWorkspaceFoldersStub.returns([workspaceFolder]); + const rootUri = Uri.file(nestedRoot); + const { showTextDocumentStub } = stubSuccessfulFileCreation(); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create({ + name: 'remote_quick', + quickCreate: true, + rootUri, + }); + + assert.ok(result); + const createdScript = result as PythonProject; + assert.strictEqual(getWorkspaceFolderStub.firstCall.args[0].toString(), rootUri.toString()); + assert.strictEqual(createdScript.uri.scheme, remoteWorkspaceUri.scheme); + assert.strictEqual(createdScript.uri.authority, remoteWorkspaceUri.authority); + assert.strictEqual(createdScript.uri.fsPath, Uri.file(path.join(nestedRoot, 'remote_quick.py')).fsPath); + assert.ok(addStub.calledOnceWithExactly(createdScript)); + assert.ok(showTextDocumentStub.calledOnceWithExactly(createdScript.uri)); + }); + + test('quick and programmatic roots outside the workspace are rejected before file creation', async () => { + const outsideRoot = Uri.file(path.join(tmpDir, 'outside')); + getWorkspaceFolderStub.returns(undefined); + getWorkspaceFoldersStub.returns([]); + sinon.stub(fsExtra, 'pathExists').resolves(true); + const copyStub = sinon.stub(fsExtra, 'copy').resolves(); + const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); + const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); + const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); + const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const quickResult = await creator.create({ + name: 'quick_outside.py', + quickCreate: true, + rootUri: outsideRoot, + }); + const programmaticResult = await creator.create({ + name: 'programmatic_outside.py', + rootUri: outsideRoot, + }); + + assert.strictEqual(quickResult, undefined); + assert.strictEqual(programmaticResult, undefined); + assert.strictEqual(getWorkspaceFolderStub.callCount, 2); + assert.strictEqual(showErrorMessageStub.callCount, 2); + assert.strictEqual(copyStub.called, false); + assert.strictEqual(replaceStub.called, false); + assert.strictEqual(addStub.called, false); + assert.strictEqual(instructionsStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + }); + + test('a destination symlink or junction resolving outside the workspace is rejected before side effects', async () => { + const workspaceRoot = path.join(tmpDir, 'workspace'); + const outsideRoot = path.join(tmpDir, 'outside'); + const linkedDestination = path.join(workspaceRoot, 'linked-outside'); + await fs.ensureDir(workspaceRoot); + await fs.ensureDir(outsideRoot); + const createdRealLink = await createDirectoryLink(outsideRoot, linkedDestination); + if (!createdRealLink) { + const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; + realpathStub.callsFake(async (targetPath: string) => { + const resolvedPath = path.resolve(targetPath); + if (resolvedPath === path.resolve(linkedDestination)) { + return path.resolve(outsideRoot); + } + if (resolvedPath === path.resolve(workspaceRoot)) { + return path.resolve(workspaceRoot); + } + throw new Error(`Unexpected realpath: ${targetPath}`); + }); + } + workspaceFolder = { + index: 0, + name: 'test-workspace', + uri: Uri.file(workspaceRoot), + }; + getWorkspaceFolderStub.returns(workspaceFolder); + getWorkspaceFoldersStub.returns([workspaceFolder]); + sinon.stub(fsExtra, 'pathExists').resolves(true); + const copyStub = sinon.stub(fsExtra, 'copy').resolves(); + const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); + const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); + const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); + const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create({ + name: 'escaped.py', + quickCreate: true, + rootUri: Uri.file(linkedDestination), + }); + + assert.strictEqual(result, undefined); + assert.ok(showErrorMessageStub.calledOnce); + assert.strictEqual(copyStub.called, false); + assert.strictEqual(replaceStub.called, false); + assert.strictEqual(addStub.called, false); + assert.strictEqual(instructionsStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + }); + + test('a symlinked workspace root remains a valid destination', async () => { + const physicalWorkspaceRoot = path.join(tmpDir, 'physical-workspace'); + const linkedWorkspaceRoot = path.join(tmpDir, 'linked-workspace'); + const nestedRoot = path.join(linkedWorkspaceRoot, 'nested'); + await fs.ensureDir(path.join(physicalWorkspaceRoot, 'nested')); + const createdRealLink = await createDirectoryLink(physicalWorkspaceRoot, linkedWorkspaceRoot); + if (!createdRealLink) { + await fs.ensureDir(nestedRoot); + const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; + realpathStub.callsFake(async (targetPath: string) => { + const resolvedPath = path.resolve(targetPath); + if (resolvedPath === path.resolve(nestedRoot)) { + return path.resolve(physicalWorkspaceRoot, 'nested'); + } + if (resolvedPath === path.resolve(linkedWorkspaceRoot)) { + return path.resolve(physicalWorkspaceRoot); + } + throw new Error(`Unexpected realpath: ${targetPath}`); + }); + } + workspaceFolder = { + index: 0, + name: 'linked-workspace', + uri: Uri.file(linkedWorkspaceRoot), + }; + getWorkspaceFolderStub.returns(workspaceFolder); + getWorkspaceFoldersStub.returns([workspaceFolder]); + const { copyStub, showTextDocumentStub } = stubSuccessfulFileCreation(); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create({ + name: 'linked_workspace.py', + quickCreate: true, + rootUri: Uri.file(nestedRoot), + }); + + assert.ok(result); + const createdScript = result as PythonProject; + assert.strictEqual(createdScript.uri.fsPath, Uri.file(path.join(nestedRoot, 'linked_workspace.py')).fsPath); + assert.ok(copyStub.calledOnce); + assert.ok(addStub.calledOnceWithExactly(createdScript)); + assert.ok(showTextDocumentStub.calledOnceWithExactly(createdScript.uri)); + }); + + test('an escaping .github symlink or junction is rejected before creator side effects', async () => { + const workspaceRoot = path.join(tmpDir, 'workspace'); + const outsideRoot = path.join(tmpDir, 'outside-github'); + const githubFolder = path.join(workspaceRoot, '.github'); + await fs.ensureDir(workspaceRoot); + await fs.ensureDir(outsideRoot); + const createdRealLink = await createDirectoryLink(outsideRoot, githubFolder); + if (!createdRealLink) { + const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; + realpathStub.callsFake(async (targetPath: string) => { + const resolvedPath = path.resolve(targetPath); + if (resolvedPath === path.resolve(githubFolder)) { + return path.resolve(outsideRoot); + } + if (resolvedPath === path.resolve(workspaceRoot)) { + return path.resolve(workspaceRoot); + } + throw new Error(`Unexpected realpath: ${targetPath}`); + }); + } + workspaceFolder = { + index: 0, + name: 'test-workspace', + uri: Uri.file(workspaceRoot), + }; + getWorkspaceFolderStub.returns(workspaceFolder); + getWorkspaceFoldersStub.returns([workspaceFolder]); + sinon.stub(fsExtra, 'pathExists').resolves(true); + const copyStub = sinon.stub(fsExtra, 'copy').resolves(); + const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); + const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); + const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); + const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create({ + name: 'blocked_github.py', + quickCreate: true, + rootUri: Uri.file(workspaceRoot), + }); + + assert.strictEqual(result, undefined); + assert.ok(showErrorMessageStub.calledOnce); + assert.strictEqual(copyStub.called, false); + assert.strictEqual(replaceStub.called, false); + assert.strictEqual(addStub.called, false); + assert.strictEqual(instructionsStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + await assert.rejects( + fs.readFile(path.join(workspaceRoot, 'blocked_github.py')), + (error: NodeJS.ErrnoException) => error.code === 'ENOENT', + ); + }); + + test('an escaping Copilot instructions file symlink is rejected before creator side effects', async () => { + const workspaceRoot = path.join(tmpDir, 'workspace'); + const githubFolder = path.join(workspaceRoot, '.github'); + const outsideInstructions = path.join(tmpDir, 'outside-copilot-instructions.md'); + const instructionsFile = path.join(githubFolder, 'copilot-instructions.md'); + await fs.ensureDir(githubFolder); + await fs.writeFile(outsideInstructions, 'outside'); + const createdRealLink = await createFileLink(outsideInstructions, instructionsFile); + if (!createdRealLink) { + const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; + realpathStub.callsFake(async (targetPath: string) => { + const resolvedPath = path.resolve(targetPath); + if (resolvedPath === path.resolve(instructionsFile)) { + return path.resolve(outsideInstructions); + } + if (resolvedPath === path.resolve(githubFolder)) { + return path.resolve(githubFolder); + } + if (resolvedPath === path.resolve(workspaceRoot)) { + return path.resolve(workspaceRoot); + } + throw new Error(`Unexpected realpath: ${targetPath}`); + }); + } + workspaceFolder = { + index: 0, + name: 'test-workspace', + uri: Uri.file(workspaceRoot), + }; + getWorkspaceFolderStub.returns(workspaceFolder); + getWorkspaceFoldersStub.returns([workspaceFolder]); + sinon.stub(fsExtra, 'pathExists').resolves(true); + const copyStub = sinon.stub(fsExtra, 'copy').resolves(); + const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); + const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); + const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); + const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create({ + name: 'blocked_instructions.py', + quickCreate: true, + rootUri: Uri.file(workspaceRoot), + }); + + assert.strictEqual(result, undefined); + assert.ok(showErrorMessageStub.calledOnce); + assert.strictEqual(copyStub.called, false); + assert.strictEqual(replaceStub.called, false); + assert.strictEqual(addStub.called, false); + assert.strictEqual(instructionsStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + }); + + test('a contained .github link and regular instructions file remain valid', async () => { + const workspaceRoot = path.join(tmpDir, 'workspace'); + const containedGithubTarget = path.join(workspaceRoot, 'contained-github'); + const githubFolder = path.join(workspaceRoot, '.github'); + await fs.ensureDir(containedGithubTarget); + await createDirectoryLink(containedGithubTarget, githubFolder); + await fs.writeFile(path.join(githubFolder, 'copilot-instructions.md'), 'existing instructions'); + workspaceFolder = { + index: 0, + name: 'test-workspace', + uri: Uri.file(workspaceRoot), + }; + getWorkspaceFolderStub.returns(workspaceFolder); + getWorkspaceFoldersStub.returns([workspaceFolder]); + const { copyStub, instructionsStub, showTextDocumentStub } = stubSuccessfulFileCreation(); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create({ + name: 'contained_instructions.py', + quickCreate: true, + rootUri: Uri.file(workspaceRoot), + }); + + assert.ok(result); + assert.ok(copyStub.calledOnce); + assert.ok(addStub.calledOnce); + assert.ok(instructionsStub.calledOnce); + assert.ok(showTextDocumentStub.calledOnce); + }); + + test('create waits for project registration before opening and returning', async () => { + const scriptFileName = 'wait_for_registration.py'; + const rootUri = Uri.file(tmpDir); + const { showTextDocumentStub } = stubSuccessfulFileCreation(); + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons'); + let notifyAddStarted!: () => void; + let releaseRegistration!: () => void; + const addStarted = new Promise((resolve) => { + notifyAddStarted = resolve; + }); + const registration = new Promise((resolve) => { + releaseRegistration = resolve; + }); + const addStub = sinon.stub().callsFake(async () => { + notifyAddStarted(); + await registration; + }); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const createPromise = creator.create({ + name: scriptFileName, + quickCreate: true, + rootUri, + }); + let createSettled = false; + const trackedCreate = createPromise.finally(() => { + createSettled = true; + }); + await addStarted; + await Promise.resolve(); + + assert.strictEqual(createSettled, false, 'create should remain pending while project registration is pending'); + assert.strictEqual(showTextDocumentStub.called, false, 'the script must not open before registration completes'); + assert.strictEqual(promptStub.called, false); + + releaseRegistration(); + const result = await trackedCreate; + + assert.ok(result); + assert.ok(addStub.calledOnce); + assert.ok(showTextDocumentStub.calledOnce); + }); + + test('an insert-then-reject registration rolls back creator side effects and preserves the error', async () => { + const rootUri = Uri.file(tmpDir); + const scriptDestination = path.resolve(rootUri.fsPath, 'registration_failure.py'); + const { instructionsStub, showTextDocumentStub } = stubSuccessfulFileCreation(); + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons'); + const projects: PythonProject[] = []; + const registrationError = new Error('registration failed after insertion'); + const addStub = sinon.stub().callsFake(async (project: PythonProject) => { + projects.push(project); + throw registrationError; + }); + const removeStub = sinon.stub().callsFake((project: PythonProject) => { + const index = projects.indexOf(project); + if (index >= 0) { + projects.splice(index, 1); + } + }); + const creator = new NewScriptProject({ + add: addStub, + remove: removeStub, + } as unknown as PythonProjectManager); + + await assert.rejects( + creator.create({ + name: 'registration_failure.py', + quickCreate: true, + rootUri, + }), + (error: unknown) => error === registrationError, + ); + + assert.ok(addStub.calledOnce); + assert.ok(removeStub.calledOnceWithExactly(addStub.firstCall.args[0])); + assert.deepStrictEqual(projects, []); + await assert.rejects(fs.readFile(scriptDestination), (error: NodeJS.ErrnoException) => error.code === 'ENOENT'); + assert.strictEqual(showTextDocumentStub.called, false); + assert.strictEqual(instructionsStub.called, false); + assert.strictEqual(promptStub.called, false); }); }); From d6413a7fbced55b73a0a69a41ba1d0944d083feb Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 14:29:49 -0700 Subject: [PATCH 2/5] Reuse shared path helper and trim symlink containment in script creator Extract isSameOrParentPath into common/utils/pathUtils and reuse it in newScriptProject and terminal/utils instead of duplicated local copies. Remove the niche fs.realpath/symlink containment preflight and isCopilotInstructionsDestinationContained while keeping input validation, remote-URI identity, logical workspace containment, and registration rollback. Update unit tests accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1487b95c-ac14-455f-9b7f-9770cf65e11e --- src/common/utils/pathUtils.ts | 22 ++ src/features/creators/newScriptProject.ts | 94 +------- src/features/terminal/utils.ts | 6 +- .../creators/newScriptProject.unit.test.ts | 219 +----------------- 4 files changed, 27 insertions(+), 314 deletions(-) diff --git a/src/common/utils/pathUtils.ts b/src/common/utils/pathUtils.ts index d398828a1..4424ab731 100644 --- a/src/common/utils/pathUtils.ts +++ b/src/common/utils/pathUtils.ts @@ -65,6 +65,28 @@ export function normalizePath(fsPath: string): string { return path1; } +/** + * Determines whether `candidatePath` is the same as, or nested inside, `parentPath`. + * + * Both paths are resolved to absolute form and normalized (case-insensitive on + * Windows) before comparison, so differences in separators or drive-letter case + * do not affect the result. + * + * @param parentPath The candidate parent (or ancestor) directory. + * @param candidatePath The path being tested for containment. + * @returns `true` when `candidatePath` equals `parentPath` or is a descendant of it. + */ +export function isSameOrParentPath(parentPath: string, candidatePath: string): boolean { + const relative = path.relative( + normalizePath(path.resolve(parentPath)), + normalizePath(path.resolve(candidatePath)), + ); + return ( + relative === '' || + (relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)) + ); +} + export function getResourceUri(resourcePath: string, root?: string): Uri | undefined { try { if (!resourcePath) { diff --git a/src/features/creators/newScriptProject.ts b/src/features/creators/newScriptProject.ts index 3056aadfb..eb806463a 100644 --- a/src/features/creators/newScriptProject.ts +++ b/src/features/creators/newScriptProject.ts @@ -4,7 +4,7 @@ import { commands, l10n, MarkdownString, QuickInputButtons, Uri, window, Workspa import { PythonProject, PythonProjectCreator, PythonProjectCreatorOptions } from '../../api'; import { NEW_PROJECT_TEMPLATES_FOLDER } from '../../common/constants'; import { traceError } from '../../common/logging'; -import { normalizePath } from '../../common/utils/pathUtils'; +import { isSameOrParentPath } from '../../common/utils/pathUtils'; import { isWindows } from '../../common/utils/platformUtils'; import { showErrorMessage, showInputBoxWithButtons, showTextDocument } from '../../common/window.apis'; import { getWorkspaceFolder, getWorkspaceFolders } from '../../common/workspace.apis'; @@ -36,17 +36,6 @@ function validateScriptFileName(value: string): string | null { return null; } -function isSameOrDescendantPath(parentPath: string, candidatePath: string): boolean { - const relativePath = path.relative( - normalizePath(path.resolve(parentPath)), - normalizePath(path.resolve(candidatePath)), - ); - return ( - relativePath === '' || - (relativePath !== '..' && !relativePath.startsWith(`..${path.sep}`) && !path.isAbsolute(relativePath)) - ); -} - function uriForFileRootInWorkspace(rootPath: string, workspaceFolder: WorkspaceFolder): Uri { const relativeRootPath = path.relative(path.resolve(workspaceFolder.uri.fsPath), path.resolve(rootPath)); const pathSegments = relativeRootPath.split(/[\\/]/).filter((segment) => segment.length > 0); @@ -55,54 +44,6 @@ function uriForFileRootInWorkspace(rootPath: string, workspaceFolder: WorkspaceF }); } -async function lstatIfExists(candidatePath: string) { - try { - return await fs.lstat(candidatePath); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - return undefined; - } - throw error; - } -} - -async function isCopilotInstructionsDestinationContained( - destinationRoot: string, - physicalDestinationRoot: string, - physicalWorkspaceRoot: string, -): Promise { - try { - const githubFolder = path.join(destinationRoot, '.github'); - const githubEntry = await lstatIfExists(githubFolder); - if (!githubEntry) { - return isSameOrDescendantPath(physicalWorkspaceRoot, physicalDestinationRoot); - } - - const physicalGithubFolder = await fs.realpath(githubFolder); - if (!isSameOrDescendantPath(physicalWorkspaceRoot, physicalGithubFolder)) { - return false; - } - if (!(await fs.stat(githubFolder)).isDirectory()) { - return false; - } - - const instructionsFile = path.join(githubFolder, 'copilot-instructions.md'); - const instructionsEntry = await lstatIfExists(instructionsFile); - if (!instructionsEntry) { - return true; - } - if (instructionsEntry.isSymbolicLink() || !instructionsEntry.isFile()) { - return false; - } - - const physicalInstructionsFile = await fs.realpath(instructionsFile); - return isSameOrDescendantPath(physicalWorkspaceRoot, physicalInstructionsFile); - } catch (error) { - traceError('Failed to validate the Copilot instructions destination:', error); - return false; - } -} - export class NewScriptProject implements PythonProjectCreator { public readonly name = l10n.t('newScript'); public readonly displayName = l10n.t('Script'); @@ -181,7 +122,7 @@ export class NewScriptProject implements PythonProjectCreator { if (!workspaceFolder && destinationRootUri.scheme === 'file') { workspaceFolders ??= getWorkspaceFolders(); workspaceFolder = workspaceFolders - ?.filter((folder) => isSameOrDescendantPath(folder.uri.fsPath, resolvedDestRoot)) + ?.filter((folder) => isSameOrParentPath(folder.uri.fsPath, resolvedDestRoot)) .sort((first, second) => second.uri.fsPath.length - first.uri.fsPath.length)[0]; } if (!workspaceFolder) { @@ -189,23 +130,6 @@ export class NewScriptProject implements PythonProjectCreator { return undefined; } - let physicalDestRoot: string; - let physicalWorkspaceRoot: string; - try { - [physicalDestRoot, physicalWorkspaceRoot] = await Promise.all([ - fs.realpath(resolvedDestRoot), - fs.realpath(workspaceFolder.uri.fsPath), - ]); - } catch (error) { - traceError('Failed to resolve the destination or workspace folder:', error); - showErrorMessage(l10n.t('Unable to resolve the destination folder inside the open workspace.')); - return undefined; - } - if (!isSameOrDescendantPath(physicalWorkspaceRoot, physicalDestRoot)) { - showErrorMessage(l10n.t('Destination folder must resolve inside the open workspace, aborting creation.')); - return undefined; - } - const identityRootUri = destinationRootUri.scheme === 'file' && workspaceFolder.uri.scheme !== 'file' ? uriForFileRootInWorkspace(resolvedDestRoot, workspaceFolder) @@ -226,20 +150,6 @@ export class NewScriptProject implements PythonProjectCreator { return undefined; } - if ( - createCopilotInstructions && - !(await isCopilotInstructionsDestinationContained( - resolvedDestRoot, - physicalDestRoot, - physicalWorkspaceRoot, - )) - ) { - showErrorMessage( - l10n.t('Copilot instructions must be stored inside the open workspace, aborting creation.'), - ); - return undefined; - } - // Check if the destination file already exists if (await fs.pathExists(scriptDestination)) { window.showErrorMessage( diff --git a/src/features/terminal/utils.ts b/src/features/terminal/utils.ts index f4320d13c..866a74861 100644 --- a/src/features/terminal/utils.ts +++ b/src/features/terminal/utils.ts @@ -11,6 +11,7 @@ import { VENV_MANAGER_ID } from '../../common/constants'; import { traceError, traceVerbose } from '../../common/logging'; import { timeout } from '../../common/utils/asyncUtils'; import { createSimpleDebounce } from '../../common/utils/debounce'; +import { isSameOrParentPath } from '../../common/utils/pathUtils'; import { onDidChangeTerminalShellIntegration, onDidWriteTerminalData } from '../../common/window.apis'; import { getConfiguration, getWorkspaceFolders } from '../../common/workspace.apis'; import { identifyTerminalShell } from '../common/shellDetector'; @@ -199,11 +200,6 @@ async function getDistinctProjectEnvs( return envs; } -function isSameOrParentPath(parentPath: string, candidatePath: string): boolean { - const relative = path.relative(path.resolve(parentPath), path.resolve(candidatePath)); - return relative === '' || (relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); -} - function getProjectForCwd(projects: readonly PythonProject[], cwd: string): PythonProject | undefined { return [...projects] .filter((project) => isSameOrParentPath(project.uri.fsPath, cwd)) diff --git a/src/test/features/creators/newScriptProject.unit.test.ts b/src/test/features/creators/newScriptProject.unit.test.ts index 128626e2b..632564004 100644 --- a/src/test/features/creators/newScriptProject.unit.test.ts +++ b/src/test/features/creators/newScriptProject.unit.test.ts @@ -87,16 +87,6 @@ suite('new723ScriptTemplate / NewScriptProject', () => { } } - async function createFileLink(target: string, link: string): Promise { - try { - await fs.symlink(target, link, 'file'); - return true; - } catch { - await fs.copyFile(target, link); - return false; - } - } - test('shipped template contains parseable PEP 723 metadata', async () => { const contents = await fs.readFile(TEMPLATE_PATH, 'utf8'); const metadata = readInlineScriptMetadata(contents); @@ -402,77 +392,13 @@ suite('new723ScriptTemplate / NewScriptProject', () => { assert.strictEqual(showTextDocumentStub.called, false); }); - test('a destination symlink or junction resolving outside the workspace is rejected before side effects', async () => { - const workspaceRoot = path.join(tmpDir, 'workspace'); - const outsideRoot = path.join(tmpDir, 'outside'); - const linkedDestination = path.join(workspaceRoot, 'linked-outside'); - await fs.ensureDir(workspaceRoot); - await fs.ensureDir(outsideRoot); - const createdRealLink = await createDirectoryLink(outsideRoot, linkedDestination); - if (!createdRealLink) { - const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; - realpathStub.callsFake(async (targetPath: string) => { - const resolvedPath = path.resolve(targetPath); - if (resolvedPath === path.resolve(linkedDestination)) { - return path.resolve(outsideRoot); - } - if (resolvedPath === path.resolve(workspaceRoot)) { - return path.resolve(workspaceRoot); - } - throw new Error(`Unexpected realpath: ${targetPath}`); - }); - } - workspaceFolder = { - index: 0, - name: 'test-workspace', - uri: Uri.file(workspaceRoot), - }; - getWorkspaceFolderStub.returns(workspaceFolder); - getWorkspaceFoldersStub.returns([workspaceFolder]); - sinon.stub(fsExtra, 'pathExists').resolves(true); - const copyStub = sinon.stub(fsExtra, 'copy').resolves(); - const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); - const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); - const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); - const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); - const addStub = sinon.stub().resolves(); - const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); - - const result = await creator.create({ - name: 'escaped.py', - quickCreate: true, - rootUri: Uri.file(linkedDestination), - }); - - assert.strictEqual(result, undefined); - assert.ok(showErrorMessageStub.calledOnce); - assert.strictEqual(copyStub.called, false); - assert.strictEqual(replaceStub.called, false); - assert.strictEqual(addStub.called, false); - assert.strictEqual(instructionsStub.called, false); - assert.strictEqual(showTextDocumentStub.called, false); - }); - test('a symlinked workspace root remains a valid destination', async () => { const physicalWorkspaceRoot = path.join(tmpDir, 'physical-workspace'); const linkedWorkspaceRoot = path.join(tmpDir, 'linked-workspace'); const nestedRoot = path.join(linkedWorkspaceRoot, 'nested'); await fs.ensureDir(path.join(physicalWorkspaceRoot, 'nested')); - const createdRealLink = await createDirectoryLink(physicalWorkspaceRoot, linkedWorkspaceRoot); - if (!createdRealLink) { - await fs.ensureDir(nestedRoot); - const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; - realpathStub.callsFake(async (targetPath: string) => { - const resolvedPath = path.resolve(targetPath); - if (resolvedPath === path.resolve(nestedRoot)) { - return path.resolve(physicalWorkspaceRoot, 'nested'); - } - if (resolvedPath === path.resolve(linkedWorkspaceRoot)) { - return path.resolve(physicalWorkspaceRoot); - } - throw new Error(`Unexpected realpath: ${targetPath}`); - }); - } + await createDirectoryLink(physicalWorkspaceRoot, linkedWorkspaceRoot); + await fs.ensureDir(nestedRoot); workspaceFolder = { index: 0, name: 'linked-workspace', @@ -498,147 +424,6 @@ suite('new723ScriptTemplate / NewScriptProject', () => { assert.ok(showTextDocumentStub.calledOnceWithExactly(createdScript.uri)); }); - test('an escaping .github symlink or junction is rejected before creator side effects', async () => { - const workspaceRoot = path.join(tmpDir, 'workspace'); - const outsideRoot = path.join(tmpDir, 'outside-github'); - const githubFolder = path.join(workspaceRoot, '.github'); - await fs.ensureDir(workspaceRoot); - await fs.ensureDir(outsideRoot); - const createdRealLink = await createDirectoryLink(outsideRoot, githubFolder); - if (!createdRealLink) { - const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; - realpathStub.callsFake(async (targetPath: string) => { - const resolvedPath = path.resolve(targetPath); - if (resolvedPath === path.resolve(githubFolder)) { - return path.resolve(outsideRoot); - } - if (resolvedPath === path.resolve(workspaceRoot)) { - return path.resolve(workspaceRoot); - } - throw new Error(`Unexpected realpath: ${targetPath}`); - }); - } - workspaceFolder = { - index: 0, - name: 'test-workspace', - uri: Uri.file(workspaceRoot), - }; - getWorkspaceFolderStub.returns(workspaceFolder); - getWorkspaceFoldersStub.returns([workspaceFolder]); - sinon.stub(fsExtra, 'pathExists').resolves(true); - const copyStub = sinon.stub(fsExtra, 'copy').resolves(); - const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); - const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); - const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); - const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); - const addStub = sinon.stub().resolves(); - const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); - - const result = await creator.create({ - name: 'blocked_github.py', - quickCreate: true, - rootUri: Uri.file(workspaceRoot), - }); - - assert.strictEqual(result, undefined); - assert.ok(showErrorMessageStub.calledOnce); - assert.strictEqual(copyStub.called, false); - assert.strictEqual(replaceStub.called, false); - assert.strictEqual(addStub.called, false); - assert.strictEqual(instructionsStub.called, false); - assert.strictEqual(showTextDocumentStub.called, false); - await assert.rejects( - fs.readFile(path.join(workspaceRoot, 'blocked_github.py')), - (error: NodeJS.ErrnoException) => error.code === 'ENOENT', - ); - }); - - test('an escaping Copilot instructions file symlink is rejected before creator side effects', async () => { - const workspaceRoot = path.join(tmpDir, 'workspace'); - const githubFolder = path.join(workspaceRoot, '.github'); - const outsideInstructions = path.join(tmpDir, 'outside-copilot-instructions.md'); - const instructionsFile = path.join(githubFolder, 'copilot-instructions.md'); - await fs.ensureDir(githubFolder); - await fs.writeFile(outsideInstructions, 'outside'); - const createdRealLink = await createFileLink(outsideInstructions, instructionsFile); - if (!createdRealLink) { - const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; - realpathStub.callsFake(async (targetPath: string) => { - const resolvedPath = path.resolve(targetPath); - if (resolvedPath === path.resolve(instructionsFile)) { - return path.resolve(outsideInstructions); - } - if (resolvedPath === path.resolve(githubFolder)) { - return path.resolve(githubFolder); - } - if (resolvedPath === path.resolve(workspaceRoot)) { - return path.resolve(workspaceRoot); - } - throw new Error(`Unexpected realpath: ${targetPath}`); - }); - } - workspaceFolder = { - index: 0, - name: 'test-workspace', - uri: Uri.file(workspaceRoot), - }; - getWorkspaceFolderStub.returns(workspaceFolder); - getWorkspaceFoldersStub.returns([workspaceFolder]); - sinon.stub(fsExtra, 'pathExists').resolves(true); - const copyStub = sinon.stub(fsExtra, 'copy').resolves(); - const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); - const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); - const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); - const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); - const addStub = sinon.stub().resolves(); - const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); - - const result = await creator.create({ - name: 'blocked_instructions.py', - quickCreate: true, - rootUri: Uri.file(workspaceRoot), - }); - - assert.strictEqual(result, undefined); - assert.ok(showErrorMessageStub.calledOnce); - assert.strictEqual(copyStub.called, false); - assert.strictEqual(replaceStub.called, false); - assert.strictEqual(addStub.called, false); - assert.strictEqual(instructionsStub.called, false); - assert.strictEqual(showTextDocumentStub.called, false); - }); - - test('a contained .github link and regular instructions file remain valid', async () => { - const workspaceRoot = path.join(tmpDir, 'workspace'); - const containedGithubTarget = path.join(workspaceRoot, 'contained-github'); - const githubFolder = path.join(workspaceRoot, '.github'); - await fs.ensureDir(containedGithubTarget); - await createDirectoryLink(containedGithubTarget, githubFolder); - await fs.writeFile(path.join(githubFolder, 'copilot-instructions.md'), 'existing instructions'); - workspaceFolder = { - index: 0, - name: 'test-workspace', - uri: Uri.file(workspaceRoot), - }; - getWorkspaceFolderStub.returns(workspaceFolder); - getWorkspaceFoldersStub.returns([workspaceFolder]); - const { copyStub, instructionsStub, showTextDocumentStub } = stubSuccessfulFileCreation(); - const addStub = sinon.stub().resolves(); - const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); - - const result = await creator.create({ - name: 'contained_instructions.py', - quickCreate: true, - rootUri: Uri.file(workspaceRoot), - }); - - assert.ok(result); - assert.ok(copyStub.calledOnce); - assert.ok(addStub.calledOnce); - assert.ok(instructionsStub.calledOnce); - assert.ok(showTextDocumentStub.calledOnce); - }); - test('create waits for project registration before opening and returning', async () => { const scriptFileName = 'wait_for_registration.py'; const rootUri = Uri.file(tmpDir); From 2074d10e6bc1ea05df71e1f4fc95e5b9fae17b3d Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 15:04:35 -0700 Subject: [PATCH 3/5] Restore physical-path containment check before writing script Resolve the destination and workspace roots with fs.realpath and reject when the destination physically escapes the workspace (e.g. via a symlink or junction) before fs.copy. Addresses review feedback that the previous containment check was lexical only. Adds back a unit test for the symlink-escape rejection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1487b95c-ac14-455f-9b7f-9770cf65e11e --- src/features/creators/newScriptProject.ts | 17 ++++++ .../creators/newScriptProject.unit.test.ts | 52 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/src/features/creators/newScriptProject.ts b/src/features/creators/newScriptProject.ts index eb806463a..82385b3c6 100644 --- a/src/features/creators/newScriptProject.ts +++ b/src/features/creators/newScriptProject.ts @@ -130,6 +130,23 @@ export class NewScriptProject implements PythonProjectCreator { return undefined; } + let physicalDestRoot: string; + let physicalWorkspaceRoot: string; + try { + [physicalDestRoot, physicalWorkspaceRoot] = await Promise.all([ + fs.realpath(resolvedDestRoot), + fs.realpath(workspaceFolder.uri.fsPath), + ]); + } catch (error) { + traceError('Failed to resolve the destination or workspace folder:', error); + showErrorMessage(l10n.t('Unable to resolve the destination folder inside the open workspace.')); + return undefined; + } + if (!isSameOrParentPath(physicalWorkspaceRoot, physicalDestRoot)) { + showErrorMessage(l10n.t('Destination folder must resolve inside the open workspace, aborting creation.')); + return undefined; + } + const identityRootUri = destinationRootUri.scheme === 'file' && workspaceFolder.uri.scheme !== 'file' ? uriForFileRootInWorkspace(resolvedDestRoot, workspaceFolder) diff --git a/src/test/features/creators/newScriptProject.unit.test.ts b/src/test/features/creators/newScriptProject.unit.test.ts index 632564004..53d175efe 100644 --- a/src/test/features/creators/newScriptProject.unit.test.ts +++ b/src/test/features/creators/newScriptProject.unit.test.ts @@ -424,6 +424,58 @@ suite('new723ScriptTemplate / NewScriptProject', () => { assert.ok(showTextDocumentStub.calledOnceWithExactly(createdScript.uri)); }); + test('a destination that physically escapes the workspace is rejected before writing', async () => { + const workspaceRoot = path.join(tmpDir, 'workspace'); + const outsideRoot = path.join(tmpDir, 'outside'); + const linkedDestination = path.join(workspaceRoot, 'linked-outside'); + await fs.ensureDir(workspaceRoot); + await fs.ensureDir(outsideRoot); + + // Simulate a destination that is lexically inside the workspace but whose + // real path (via a symlink/junction) resolves outside of it. + const realpathStub = sinon.stub(fsExtra, 'realpath') as sinon.SinonStub; + realpathStub.callsFake(async (targetPath: string) => { + const resolved = path.resolve(String(targetPath)); + if (path.relative(resolved, path.resolve(linkedDestination)) === '') { + return path.resolve(outsideRoot); + } + if (path.relative(resolved, path.resolve(workspaceRoot)) === '') { + return path.resolve(workspaceRoot); + } + throw new Error(`Unexpected realpath: ${targetPath}`); + }); + + workspaceFolder = { + index: 0, + name: 'test-workspace', + uri: Uri.file(workspaceRoot), + }; + getWorkspaceFolderStub.returns(workspaceFolder); + getWorkspaceFoldersStub.returns([workspaceFolder]); + sinon.stub(fsExtra, 'pathExists').resolves(true); + const copyStub = sinon.stub(fsExtra, 'copy').resolves(); + const replaceStub = sinon.stub(creationHelpers, 'replaceInFilesAndNames').resolves(); + const instructionsStub = sinon.stub(creationHelpers, 'manageCopilotInstructionsFile').resolves(); + const showTextDocumentStub = sinon.stub(windowApis, 'showTextDocument'); + const showErrorMessageStub = sinon.stub(windowApis, 'showErrorMessage'); + const addStub = sinon.stub().resolves(); + const creator = new NewScriptProject({ add: addStub } as unknown as PythonProjectManager); + + const result = await creator.create({ + name: 'escaped.py', + quickCreate: true, + rootUri: Uri.file(linkedDestination), + }); + + assert.strictEqual(result, undefined); + assert.ok(showErrorMessageStub.calledOnce); + assert.strictEqual(copyStub.called, false); + assert.strictEqual(replaceStub.called, false); + assert.strictEqual(addStub.called, false); + assert.strictEqual(instructionsStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + }); + test('create waits for project registration before opening and returning', async () => { const scriptFileName = 'wait_for_registration.py'; const rootUri = Uri.file(tmpDir); From 5820d75c4f97661d793ae279d102116497b4af14 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 15:28:32 -0700 Subject: [PATCH 4/5] Roll back partial script on substitution failure and harden test stub Wrap the template copy, name substitution, and project registration in a single cleanup boundary so a failure after fs.copy removes the partially created script instead of leaving it behind and blocking a clean retry. Make the test fixture's pathExists fake resolve by the requested path rather than by call order, and add a regression test for the substitution-failure rollback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1487b95c-ac14-455f-9b7f-9770cf65e11e --- src/features/creators/newScriptProject.ts | 31 ++++++----- .../creators/newScriptProject.unit.test.ts | 52 +++++++++++++++++-- 2 files changed, 67 insertions(+), 16 deletions(-) diff --git a/src/features/creators/newScriptProject.ts b/src/features/creators/newScriptProject.ts index 82385b3c6..ab7030405 100644 --- a/src/features/creators/newScriptProject.ts +++ b/src/features/creators/newScriptProject.ts @@ -176,32 +176,37 @@ export class NewScriptProject implements PythonProjectCreator { ); return undefined; } - await fs.copy(newScriptTemplateFile, scriptDestination); - - // 2. Replace 'script_name' in the file using a helper (just script name remove .py) - await replaceInFilesAndNames(scriptDestination, 'script_name', scriptFileName.replace(/\.py$/, '')); - - // Add the created script to the project manager + // Build the project entry up front so copying the template, substituting + // the script name, and registering the project share one cleanup boundary: + // if any step fails, the partially created script is removed so a retry + // starts from a clean state. const createdScript: PythonProject = { name: scriptFileName, uri: identityRootUri.with({ path: path.posix.join(identityRootUri.path, scriptFileName), }), }; + let projectRegistrationAttempted = false; try { + await fs.copy(newScriptTemplateFile, scriptDestination); + // Replace 'script_name' in the file (script name without the .py suffix). + await replaceInFilesAndNames(scriptDestination, 'script_name', scriptFileName.replace(/\.py$/, '')); + projectRegistrationAttempted = true; await this.projectManager.add(createdScript); - } catch (registrationError) { - try { - this.projectManager.remove(createdScript); - } catch (rollbackError) { - traceError('Failed to remove the new script project after registration failed:', rollbackError); + } catch (creationError) { + if (projectRegistrationAttempted) { + try { + this.projectManager.remove(createdScript); + } catch (rollbackError) { + traceError('Failed to remove the new script project after creation failed:', rollbackError); + } } try { await fs.remove(scriptDestination); } catch (rollbackError) { - traceError('Failed to delete the new script after registration failed:', rollbackError); + traceError('Failed to delete the new script after creation failed:', rollbackError); } - throw registrationError; + throw creationError; } // 3. add custom github copilot instructions diff --git a/src/test/features/creators/newScriptProject.unit.test.ts b/src/test/features/creators/newScriptProject.unit.test.ts index 53d175efe..3bbec7374 100644 --- a/src/test/features/creators/newScriptProject.unit.test.ts +++ b/src/test/features/creators/newScriptProject.unit.test.ts @@ -63,12 +63,17 @@ suite('new723ScriptTemplate / NewScriptProject', () => { }); function stubSuccessfulFileCreation() { + const templateFile = path.resolve( + path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'new723ScriptTemplate', 'script.py'), + ); const showTextDocumentStub = sinon .stub(windowApis, 'showTextDocument') .resolves({} as TextEditor); - const pathExistsStub = sinon.stub(fsExtra, 'pathExists'); - pathExistsStub.onFirstCall().resolves(true); - pathExistsStub.onSecondCall().resolves(false); + // Resolve existence by the requested path (the template exists, the new + // script does not) so the fixture does not depend on probe call order. + sinon.stub(fsExtra, 'pathExists').callsFake(async (checkedPath) => { + return path.resolve(String(checkedPath)) === templateFile; + }); const copyStub = sinon.stub(fsExtra, 'copy').callsFake(async (_source, destination) => { await fs.copyFile(TEMPLATE_PATH, destination); }); @@ -558,4 +563,45 @@ suite('new723ScriptTemplate / NewScriptProject', () => { assert.strictEqual(instructionsStub.called, false); assert.strictEqual(promptStub.called, false); }); + + test('a substitution failure after copy removes the partially created script', async () => { + const rootUri = Uri.file(tmpDir); + const scriptDestination = path.resolve(rootUri.fsPath, 'substitution_failure.py'); + const { copyStub, instructionsStub, replaceStub, showTextDocumentStub } = stubSuccessfulFileCreation(); + const substitutionError = new Error('template substitution failed'); + replaceStub.rejects(substitutionError); + const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons'); + const addStub = sinon.stub().resolves(); + const removeStub = sinon.stub(); + const creator = new NewScriptProject({ + add: addStub, + remove: removeStub, + } as unknown as PythonProjectManager); + + await assert.rejects( + creator.create({ + name: 'substitution_failure.py', + quickCreate: true, + rootUri, + }), + (error: unknown) => error === substitutionError, + ); + + assert.ok(copyStub.calledOnce, 'the template must be copied before substitution runs'); + assert.ok(replaceStub.calledOnce); + await assert.rejects( + fs.readFile(scriptDestination), + (error: NodeJS.ErrnoException) => error.code === 'ENOENT', + 'the copied script must be removed when substitution fails', + ); + assert.strictEqual(addStub.called, false, 'registration must not run when substitution fails'); + assert.strictEqual( + removeStub.called, + false, + 'project rollback must not run when registration was never attempted', + ); + assert.strictEqual(instructionsStub.called, false); + assert.strictEqual(showTextDocumentStub.called, false); + assert.strictEqual(promptStub.called, false); + }); }); From 908f38e0d2d35b0a89c31303d47e105c738afa22 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 26 Aug 2026 10:41:29 -0700 Subject: [PATCH 5/5] Extract Windows device-name check and rename script template Move the Windows reserved-device-name test out of the script filename validator and into a shared isWindowsReservedDeviceName helper in common/utils/pathUtils, so the rule lives with the other path utilities and can be reused. newScriptProject now calls the helper and no longer imports isWindows directly. Add focused unit coverage for the helper. Rename the internal template folder new723ScriptTemplate to newInlineScriptTemplate and update all creator and test references. Only the folder name changes; the template content and the user-facing creator, class, and command are untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1487b95c-ac14-455f-9b7f-9770cf65e11e --- .../script.py | 0 src/common/utils/pathUtils.ts | 16 ++++++++ src/features/creators/newScriptProject.ts | 8 ++-- src/test/common/pathUtils.unit.test.ts | 38 ++++++++++++++++++- .../creators/newScriptProject.unit.test.ts | 8 ++-- 5 files changed, 60 insertions(+), 10 deletions(-) rename files/templates/{new723ScriptTemplate => newInlineScriptTemplate}/script.py (100%) diff --git a/files/templates/new723ScriptTemplate/script.py b/files/templates/newInlineScriptTemplate/script.py similarity index 100% rename from files/templates/new723ScriptTemplate/script.py rename to files/templates/newInlineScriptTemplate/script.py diff --git a/src/common/utils/pathUtils.ts b/src/common/utils/pathUtils.ts index 4424ab731..df796e5f7 100644 --- a/src/common/utils/pathUtils.ts +++ b/src/common/utils/pathUtils.ts @@ -87,6 +87,22 @@ export function isSameOrParentPath(parentPath: string, candidatePath: string): b ); } +/** + * Determines whether `value` maps to a reserved Windows device name (e.g. `CON`, + * `PRN`, `AUX`, `NUL`, `COM1`-`COM9`, `LPT1`-`LPT9`). + * + * The check inspects the portion of the name before the first dot, since Windows + * disallows these names regardless of extension, and only reports `true` on + * Windows, where the restriction applies. + * + * @param value The base file name (without directory) to test. + * @returns `true` on Windows when `value` resolves to a reserved device name. + */ +export function isWindowsReservedDeviceName(value: string): boolean { + const deviceBaseName = value.split('.')[0]; + return isWindows() && /^(con|prn|aux|nul|com[1-9]|lpt[1-9])$/i.test(deviceBaseName); +} + export function getResourceUri(resourcePath: string, root?: string): Uri | undefined { try { if (!resourcePath) { diff --git a/src/features/creators/newScriptProject.ts b/src/features/creators/newScriptProject.ts index ab7030405..93196145a 100644 --- a/src/features/creators/newScriptProject.ts +++ b/src/features/creators/newScriptProject.ts @@ -4,8 +4,7 @@ import { commands, l10n, MarkdownString, QuickInputButtons, Uri, window, Workspa import { PythonProject, PythonProjectCreator, PythonProjectCreatorOptions } from '../../api'; import { NEW_PROJECT_TEMPLATES_FOLDER } from '../../common/constants'; import { traceError } from '../../common/logging'; -import { isSameOrParentPath } from '../../common/utils/pathUtils'; -import { isWindows } from '../../common/utils/platformUtils'; +import { isSameOrParentPath, isWindowsReservedDeviceName } from '../../common/utils/pathUtils'; import { showErrorMessage, showInputBoxWithButtons, showTextDocument } from '../../common/window.apis'; import { getWorkspaceFolder, getWorkspaceFolders } from '../../common/workspace.apis'; import { PythonProjectManager } from '../../internal.api'; @@ -20,8 +19,7 @@ function validateScriptFileName(value: string): string | null { return l10n.t('Script name must end with ".py".'); } const baseName = value.replace(/\.py$/, ''); - const deviceBaseName = baseName.split('.')[0]; - if (isWindows() && /^(con|prn|aux|nul|com[1-9]|lpt[1-9])$/i.test(deviceBaseName)) { + if (isWindowsReservedDeviceName(baseName)) { return l10n.t('Script name uses a reserved Windows device name.'); } // following PyPI (PEP 508) rules for package names @@ -97,7 +95,7 @@ export class NewScriptProject implements PythonProjectCreator { } // 1. Copy template file - const newScriptTemplateFile = path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'new723ScriptTemplate', 'script.py'); + const newScriptTemplateFile = path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'newInlineScriptTemplate', 'script.py'); if (!(await fs.pathExists(newScriptTemplateFile))) { window.showErrorMessage(l10n.t('Template file does not exist, aborting creation.')); traceError(`Template file not found at: ${newScriptTemplateFile}`); diff --git a/src/test/common/pathUtils.unit.test.ts b/src/test/common/pathUtils.unit.test.ts index 1733e789f..c9bdd6150 100644 --- a/src/test/common/pathUtils.unit.test.ts +++ b/src/test/common/pathUtils.unit.test.ts @@ -1,7 +1,7 @@ import assert from 'node:assert'; import * as sinon from 'sinon'; import { Uri } from 'vscode'; -import { getResourceUri, normalizePath } from '../../common/utils/pathUtils'; +import { getResourceUri, isWindowsReservedDeviceName, normalizePath } from '../../common/utils/pathUtils'; import * as utils from '../../common/utils/platformUtils'; suite('Path Utilities', () => { @@ -128,4 +128,40 @@ suite('Path Utilities', () => { assert.strictEqual(result, 'C:/Path/To/File.txt'); }); }); + + suite('isWindowsReservedDeviceName', () => { + let isWindowsStub: sinon.SinonStub; + + setup(() => { + isWindowsStub = sinon.stub(utils, 'isWindows'); + }); + + teardown(() => { + sinon.restore(); + }); + + test('flags reserved device names on Windows regardless of extension', () => { + isWindowsStub.returns(true); + + for (const name of ['CON', 'prn', 'aux', 'nul', 'com1', 'LPT9', 'con.py', 'nul.foo']) { + assert.strictEqual(isWindowsReservedDeviceName(name), true, `${name} should be reserved`); + } + }); + + test('allows names that only resemble reserved device names on Windows', () => { + isWindowsStub.returns(true); + + for (const name of ['console', 'com10', 'lpt0', 'printer', 'aux_helper']) { + assert.strictEqual(isWindowsReservedDeviceName(name), false, `${name} should be allowed`); + } + }); + + test('never flags reserved device names off Windows', () => { + isWindowsStub.returns(false); + + for (const name of ['CON', 'nul', 'com1', 'lpt9']) { + assert.strictEqual(isWindowsReservedDeviceName(name), false, `${name} should be allowed off Windows`); + } + }); + }); }); diff --git a/src/test/features/creators/newScriptProject.unit.test.ts b/src/test/features/creators/newScriptProject.unit.test.ts index 3bbec7374..02f7ebb9e 100644 --- a/src/test/features/creators/newScriptProject.unit.test.ts +++ b/src/test/features/creators/newScriptProject.unit.test.ts @@ -28,7 +28,7 @@ const TEMPLATE_PATH = path.join( '..', 'files', 'templates', - 'new723ScriptTemplate', + 'newInlineScriptTemplate', 'script.py', ); @@ -40,7 +40,7 @@ function asRemoteUri(fsPath: string, authority = 'ssh-remote+test-host'): Uri { }); } -suite('new723ScriptTemplate / NewScriptProject', () => { +suite('newInlineScriptTemplate / NewScriptProject', () => { let tmpDir: string; let getWorkspaceFolderStub: sinon.SinonStub; let getWorkspaceFoldersStub: sinon.SinonStub; @@ -64,7 +64,7 @@ suite('new723ScriptTemplate / NewScriptProject', () => { function stubSuccessfulFileCreation() { const templateFile = path.resolve( - path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'new723ScriptTemplate', 'script.py'), + path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'newInlineScriptTemplate', 'script.py'), ); const showTextDocumentStub = sinon .stub(windowApis, 'showTextDocument') @@ -250,7 +250,7 @@ suite('new723ScriptTemplate / NewScriptProject', () => { const scriptDestination = path.resolve(rootUri.fsPath, scriptFileName); const expectedTemplatePath = path.join( NEW_PROJECT_TEMPLATES_FOLDER, - 'new723ScriptTemplate', + 'newInlineScriptTemplate', 'script.py', ); const addStub = sinon.stub().resolves();