Skip to content

Commit 53b0fd1

Browse files
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
1 parent 3a36798 commit 53b0fd1

2 files changed

Lines changed: 67 additions & 16 deletions

File tree

src/features/creators/newScriptProject.ts

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -176,32 +176,37 @@ export class NewScriptProject implements PythonProjectCreator {
176176
);
177177
return undefined;
178178
}
179-
await fs.copy(newScriptTemplateFile, scriptDestination);
180-
181-
// 2. Replace 'script_name' in the file using a helper (just script name remove .py)
182-
await replaceInFilesAndNames(scriptDestination, 'script_name', scriptFileName.replace(/\.py$/, ''));
183-
184-
// Add the created script to the project manager
179+
// Build the project entry up front so copying the template, substituting
180+
// the script name, and registering the project share one cleanup boundary:
181+
// if any step fails, the partially created script is removed so a retry
182+
// starts from a clean state.
185183
const createdScript: PythonProject = {
186184
name: scriptFileName,
187185
uri: identityRootUri.with({
188186
path: path.posix.join(identityRootUri.path, scriptFileName),
189187
}),
190188
};
189+
let projectRegistrationAttempted = false;
191190
try {
191+
await fs.copy(newScriptTemplateFile, scriptDestination);
192+
// Replace 'script_name' in the file (script name without the .py suffix).
193+
await replaceInFilesAndNames(scriptDestination, 'script_name', scriptFileName.replace(/\.py$/, ''));
194+
projectRegistrationAttempted = true;
192195
await this.projectManager.add(createdScript);
193-
} catch (registrationError) {
194-
try {
195-
this.projectManager.remove(createdScript);
196-
} catch (rollbackError) {
197-
traceError('Failed to remove the new script project after registration failed:', rollbackError);
196+
} catch (creationError) {
197+
if (projectRegistrationAttempted) {
198+
try {
199+
this.projectManager.remove(createdScript);
200+
} catch (rollbackError) {
201+
traceError('Failed to remove the new script project after creation failed:', rollbackError);
202+
}
198203
}
199204
try {
200205
await fs.remove(scriptDestination);
201206
} catch (rollbackError) {
202-
traceError('Failed to delete the new script after registration failed:', rollbackError);
207+
traceError('Failed to delete the new script after creation failed:', rollbackError);
203208
}
204-
throw registrationError;
209+
throw creationError;
205210
}
206211

207212
// 3. add custom github copilot instructions

src/test/features/creators/newScriptProject.unit.test.ts

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,17 @@ suite('new723ScriptTemplate / NewScriptProject', () => {
6363
});
6464

6565
function stubSuccessfulFileCreation() {
66+
const templateFile = path.resolve(
67+
path.join(NEW_PROJECT_TEMPLATES_FOLDER, 'new723ScriptTemplate', 'script.py'),
68+
);
6669
const showTextDocumentStub = sinon
6770
.stub(windowApis, 'showTextDocument')
6871
.resolves({} as TextEditor);
69-
const pathExistsStub = sinon.stub(fsExtra, 'pathExists');
70-
pathExistsStub.onFirstCall().resolves(true);
71-
pathExistsStub.onSecondCall().resolves(false);
72+
// Resolve existence by the requested path (the template exists, the new
73+
// script does not) so the fixture does not depend on probe call order.
74+
sinon.stub(fsExtra, 'pathExists').callsFake(async (checkedPath) => {
75+
return path.resolve(String(checkedPath)) === templateFile;
76+
});
7277
const copyStub = sinon.stub(fsExtra, 'copy').callsFake(async (_source, destination) => {
7378
await fs.copyFile(TEMPLATE_PATH, destination);
7479
});
@@ -558,4 +563,45 @@ suite('new723ScriptTemplate / NewScriptProject', () => {
558563
assert.strictEqual(instructionsStub.called, false);
559564
assert.strictEqual(promptStub.called, false);
560565
});
566+
567+
test('a substitution failure after copy removes the partially created script', async () => {
568+
const rootUri = Uri.file(tmpDir);
569+
const scriptDestination = path.resolve(rootUri.fsPath, 'substitution_failure.py');
570+
const { copyStub, instructionsStub, replaceStub, showTextDocumentStub } = stubSuccessfulFileCreation();
571+
const substitutionError = new Error('template substitution failed');
572+
replaceStub.rejects(substitutionError);
573+
const promptStub = sinon.stub(windowApis, 'showInputBoxWithButtons');
574+
const addStub = sinon.stub().resolves();
575+
const removeStub = sinon.stub();
576+
const creator = new NewScriptProject({
577+
add: addStub,
578+
remove: removeStub,
579+
} as unknown as PythonProjectManager);
580+
581+
await assert.rejects(
582+
creator.create({
583+
name: 'substitution_failure.py',
584+
quickCreate: true,
585+
rootUri,
586+
}),
587+
(error: unknown) => error === substitutionError,
588+
);
589+
590+
assert.ok(copyStub.calledOnce, 'the template must be copied before substitution runs');
591+
assert.ok(replaceStub.calledOnce);
592+
await assert.rejects(
593+
fs.readFile(scriptDestination),
594+
(error: NodeJS.ErrnoException) => error.code === 'ENOENT',
595+
'the copied script must be removed when substitution fails',
596+
);
597+
assert.strictEqual(addStub.called, false, 'registration must not run when substitution fails');
598+
assert.strictEqual(
599+
removeStub.called,
600+
false,
601+
'project rollback must not run when registration was never attempted',
602+
);
603+
assert.strictEqual(instructionsStub.called, false);
604+
assert.strictEqual(showTextDocumentStub.called, false);
605+
assert.strictEqual(promptStub.called, false);
606+
});
561607
});

0 commit comments

Comments
 (0)