Skip to content

Commit 897bc3b

Browse files
fix: fail gracefully on invalid pyproject.toml (Fixes #411) (#1942)
## Summary - preserve `pyproject.toml` read and parse failures instead of treating malformed files as empty - validate the project, build-system, dependency container, and optional-extra fields consumed during installation - preflight only the exact dependency files selected by configurable creation while Quick Create validates relevant workspace dependencies - apply validation-independent preferred-root eligibility before duplicate selection so sibling worktrees cannot block or replace the owning project - preserve exact owner/ancestor precedence even when malformed metadata cannot yet emit install arguments - stop before environment creation on Open, Cancel, or dismissal; Continue Anyway uses only the filtered owning installables ## Validation - `npm run lint` - `npm run compile-tests` - `npm run compile` - `npm run unittest` — 3,334 passing, 9 pending - focused pyproject/creation suite — 179 passing - VS Code smoke tests — 31 passing, 1 pending - VS Code E2E tests — 4 passing - disposable VS Code integration workflows — syntax errors, schema errors, invalid extras, sibling worktrees in both directions, no `.venv` on rejection, and valid creation/removal all passed - broad integration suite — 60 passing, 8 pending; the existing system-manager creation fixture timed out waiting for interactive Python installation - final Copilot review — approval recommended, no open issues Fixes #411 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 34d0c32e-e8b7-4711-a695-1b6f6e56503f
1 parent 4f44915 commit 897bc3b

8 files changed

Lines changed: 1121 additions & 164 deletions

File tree

‎src/managers/builtin/pipUtils.ts‎

Lines changed: 237 additions & 110 deletions
Large diffs are not rendered by default.

‎src/managers/builtin/venvStepBasedFlow.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -351,8 +351,9 @@ export async function createStepBasedVenvFlow(
351351
allPackages.push(...options.additionalPackages);
352352
}
353353

354-
const validationError = result.validationError;
355-
const shouldProceed = await shouldProceedAfterPyprojectValidation(validationError, allPackages);
354+
const shouldProceed = await shouldProceedAfterPyprojectValidation(result.validationErrors, {
355+
allPyprojectTomlsSelected: true,
356+
});
356357
if (!shouldProceed) {
357358
return undefined;
358359
}

‎src/managers/builtin/venvUtils.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -615,7 +615,7 @@ export async function quickCreateVenv(
615615
allPackages.push(...additionalPackages);
616616
}
617617

618-
const validationError = result.validationError;
618+
const validationError = result.validationErrors[0];
619619
if (operation && validationError) {
620620
throw new PythonToolError(
621621
'INVALID_PROJECT',
@@ -626,7 +626,11 @@ export async function quickCreateVenv(
626626
),
627627
);
628628
}
629-
const shouldProceed = operation || (await shouldProceedAfterPyprojectValidation(validationError, allPackages));
629+
const shouldProceed =
630+
operation ||
631+
(await shouldProceedAfterPyprojectValidation(result.validationErrors, {
632+
allPyprojectTomlsSelected: true,
633+
}));
630634
if (!shouldProceed) {
631635
return undefined;
632636
}

‎src/managers/common/pickers.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ function groupByInstalled(items: PackageQuickPickItem[], installed?: string[]):
108108
interface PackagesPickerResult {
109109
install: string[];
110110
uninstall: string[];
111+
selectedUris?: Uri[];
111112
}
112113

113114
function selectionsToResult(selections: string[], installed: string[]): PackagesPickerResult {
@@ -280,11 +281,12 @@ export async function selectFromInstallableToInstall(
280281
);
281282

282283
if (selected) {
283-
if (Array.isArray(selected)) {
284-
return { install: selected.flatMap((s) => s.args ?? []), uninstall: [] };
285-
} else {
286-
return { install: selected.args ?? [], uninstall: [] };
287-
}
284+
const selectedItems = Array.isArray(selected) ? selected : [selected];
285+
return {
286+
install: selectedItems.flatMap((item) => item.args ?? []),
287+
uninstall: [],
288+
selectedUris: selectedItems.flatMap((item) => (item.uri ? [item.uri] : [])),
289+
};
288290
}
289291
return undefined;
290292
}

‎src/test/managers/builtin/helpers.validatePyproject.unit.test.ts‎

Lines changed: 137 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import assert from 'assert';
2+
import * as path from 'path';
3+
import * as sinon from 'sinon';
24
import { Uri } from 'vscode';
5+
import * as windowApis from '../../../common/window.apis';
36
import {
47
PyprojectToml,
58
shouldProceedAfterPyprojectValidation,
@@ -9,66 +12,103 @@ import {
912

1013
suite('pipUtils - validatePyproject', () => {
1114
suite('shouldProceedAfterPyprojectValidation', () => {
15+
const invalidRoot = path.join(process.cwd(), 'test-data', 'invalid-project');
16+
const otherRoot = path.join(process.cwd(), 'test-data', 'other-project');
1217
const mockValidationError: ValidationError = {
1318
message: 'Invalid package name "my package" in pyproject.toml.',
14-
fileUri: Uri.file('/test/path/pyproject.toml'),
19+
fileUri: Uri.file(path.join(invalidRoot, 'pyproject.toml')),
1520
};
21+
let showErrorMessage: sinon.SinonStub;
22+
let showTextDocument: sinon.SinonStub;
1623

17-
test('should return true when no validation error exists', async () => {
18-
// Arrange: no validation error
19-
const validationError = undefined;
20-
const install = ['-e', '/test/path'];
24+
setup(() => {
25+
showErrorMessage = sinon.stub(windowApis, 'showErrorMessage');
26+
showTextDocument = sinon.stub(windowApis, 'showTextDocument').resolves({} as never);
27+
});
2128

22-
// Act
23-
const result = await shouldProceedAfterPyprojectValidation(validationError, install);
29+
teardown(() => sinon.restore());
2430

25-
// Assert
26-
assert.strictEqual(result, true, 'Should proceed when no validation error');
31+
test('returns true when no validation error exists', async () => {
32+
assert.strictEqual(await shouldProceedAfterPyprojectValidation([]), true);
33+
assert.ok(showErrorMessage.notCalled);
2734
});
2835

29-
test('should return true when install array is empty', async () => {
30-
// Arrange: validation error exists but no packages selected
31-
const install: string[] = [];
36+
test('does not block when no pyproject.toml or a different file was selected', async () => {
37+
assert.strictEqual(await shouldProceedAfterPyprojectValidation([mockValidationError]), true);
38+
assert.strictEqual(
39+
await shouldProceedAfterPyprojectValidation([mockValidationError], {
40+
selectedPyprojectTomls: [Uri.file(path.join(otherRoot, 'pyproject.toml'))],
41+
}),
42+
true,
43+
);
44+
assert.ok(showErrorMessage.notCalled);
45+
});
3246

33-
// Act
34-
const result = await shouldProceedAfterPyprojectValidation(mockValidationError, install);
47+
test('does not confuse a bracketed sibling directory with extras from the invalid project', async () => {
48+
const bracketedSibling = Uri.file(`${invalidRoot}[docs]` + path.sep + 'pyproject.toml');
49+
50+
const result = await shouldProceedAfterPyprojectValidation([mockValidationError], {
51+
selectedPyprojectTomls: [bracketedSibling],
52+
});
3553

36-
// Assert
37-
assert.strictEqual(result, true, 'Should proceed when no packages selected');
54+
assert.strictEqual(result, true);
55+
assert.ok(showErrorMessage.notCalled);
3856
});
3957

40-
test('should return true when only requirements.txt packages selected (no -e flag)', async () => {
41-
// Arrange: validation error exists but only requirements.txt packages selected
42-
const install = ['-r', '/test/requirements.txt'];
58+
test('continues when Continue Anyway is selected for the matching pyproject', async () => {
59+
showErrorMessage.callsFake(async (...args: unknown[]) => args[2] as never);
4360

44-
// Act
45-
const result = await shouldProceedAfterPyprojectValidation(mockValidationError, install);
61+
const result = await shouldProceedAfterPyprojectValidation([mockValidationError], {
62+
selectedPyprojectTomls: [mockValidationError.fileUri],
63+
});
4664

47-
// Assert
48-
assert.strictEqual(result, true, 'Should proceed when no TOML packages selected');
65+
assert.strictEqual(result, true);
66+
assert.ok(showErrorMessage.calledOnce);
67+
assert.ok(showTextDocument.notCalled);
4968
});
5069

51-
test('should return true when only PyPI packages selected (no flags at all)', async () => {
52-
// Arrange: only PyPI package names, no flags
53-
const install = ['numpy', 'pandas', 'requests'];
70+
test('opens the matching pyproject and stops creation when Open is selected', async () => {
71+
showErrorMessage.callsFake(async (...args: unknown[]) => args[1] as never);
5472

55-
// Act
56-
const result = await shouldProceedAfterPyprojectValidation(mockValidationError, install);
73+
const result = await shouldProceedAfterPyprojectValidation([mockValidationError], {
74+
selectedPyprojectTomls: [mockValidationError.fileUri],
75+
});
5776

58-
// Assert
59-
assert.strictEqual(result, true, 'Should proceed when only PyPI packages selected');
77+
assert.strictEqual(result, false);
78+
assert.ok(showTextDocument.calledOnceWithExactly(mockValidationError.fileUri));
6079
});
6180

62-
test('should not trigger on -e flag at end of array without following argument', async () => {
63-
// Arrange: -e flag is last item (malformed, but should not crash)
64-
const install = ['numpy', '-e'];
65-
// This is edge case - -e at end means no path follows, so index + 1 < arr.length is false
81+
test('stops creation without opening the file when Cancel is selected', async () => {
82+
showErrorMessage.callsFake(async (...args: unknown[]) => args[3] as never);
6683

67-
// Act
68-
const result = await shouldProceedAfterPyprojectValidation(mockValidationError, install);
84+
const result = await shouldProceedAfterPyprojectValidation([mockValidationError], {
85+
selectedPyprojectTomls: [mockValidationError.fileUri],
86+
});
6987

70-
// Assert
71-
assert.strictEqual(result, true, 'Should not crash on malformed -e flag at end');
88+
assert.strictEqual(result, false);
89+
assert.ok(showTextDocument.notCalled);
90+
});
91+
92+
test('stops creation when the validation dialog is dismissed', async () => {
93+
showErrorMessage.resolves(undefined);
94+
95+
const result = await shouldProceedAfterPyprojectValidation([mockValidationError], {
96+
selectedPyprojectTomls: [mockValidationError.fileUri],
97+
});
98+
99+
assert.strictEqual(result, false);
100+
assert.ok(showTextDocument.notCalled);
101+
});
102+
103+
test('prompts for an invalid-file-only Quick Create selection', async () => {
104+
showErrorMessage.callsFake(async (...args: unknown[]) => args[3] as never);
105+
106+
const result = await shouldProceedAfterPyprojectValidation([mockValidationError], {
107+
allPyprojectTomlsSelected: true,
108+
});
109+
110+
assert.strictEqual(result, false);
111+
assert.ok(showErrorMessage.calledOnce);
72112
});
73113
});
74114

@@ -196,6 +236,65 @@ suite('pipUtils - validatePyproject', () => {
196236
});
197237
});
198238

239+
suite('validatePyprojectToml - Dependency Values', () => {
240+
test('accepts dependency arrays containing strings', () => {
241+
verifyValidationError(
242+
{
243+
project: {
244+
name: 'test',
245+
dependencies: ['requests>=2', 'httpx'],
246+
'optional-dependencies': { test: ['pytest'] },
247+
},
248+
'build-system': { requires: ['setuptools'] },
249+
},
250+
undefined,
251+
);
252+
});
253+
254+
test('rejects project dependencies that are not an array of strings', () => {
255+
const toml = {
256+
project: { name: 'test', dependencies: 'requests' },
257+
} as PyprojectToml;
258+
assert.match(validatePyprojectToml(toml) ?? '', /dependencies.*array of strings/i);
259+
});
260+
261+
test('rejects non-string entries in project dependencies', () => {
262+
const toml = {
263+
project: { name: 'test', dependencies: ['requests', 42] },
264+
} as PyprojectToml;
265+
assert.match(validatePyprojectToml(toml) ?? '', /dependencies.*array of strings/i);
266+
});
267+
268+
test('rejects optional dependencies that are not a table', () => {
269+
const toml = {
270+
project: { name: 'test', 'optional-dependencies': ['pytest'] },
271+
} as PyprojectToml;
272+
assert.match(validatePyprojectToml(toml) ?? '', /optional-dependencies.*table/i);
273+
});
274+
275+
test('rejects invalid optional dependency group names', () => {
276+
const toml = {
277+
project: { name: 'test', 'optional-dependencies': { 'bad extra': ['pytest'] } },
278+
} as PyprojectToml;
279+
assert.match(validatePyprojectToml(toml) ?? '', /invalid optional dependency group name/i);
280+
});
281+
282+
test('rejects optional dependency groups that are not arrays of strings', () => {
283+
const toml = {
284+
project: { name: 'test', 'optional-dependencies': { test: 'pytest' } },
285+
} as PyprojectToml;
286+
assert.match(validatePyprojectToml(toml) ?? '', /group.*array of strings/i);
287+
});
288+
289+
test('rejects build-system requirements that are not arrays of strings', () => {
290+
const toml = {
291+
project: { name: 'test' },
292+
'build-system': { requires: 'setuptools' },
293+
} as PyprojectToml;
294+
assert.match(validatePyprojectToml(toml) ?? '', /requires.*array of strings/i);
295+
});
296+
});
297+
199298
suite('validatePyprojectToml - Required Fields (PEP 621)', () => {
200299
test('should accept valid project with name', () => {
201300
const toml: PyprojectToml = {

0 commit comments

Comments
 (0)