Skip to content

Commit 22b72d8

Browse files
mokagioclaude
andcommitted
Fail the Windows build if provider prune leaves a target
`pruneUnusedProviders` now returns the directories it couldn't remove, and the Forge hook throws on `win32` when any remain. A leftover provider tree there resurfaces as the very `PathTooLongException` the prune prevents, so surfacing the offending paths up front beats a later opaque Squirrel crash. Other platforms still tolerate a stuck removal — it's only a size optimization there. Addresses Copilot review on #3735. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 11f57ba commit 22b72d8

3 files changed

Lines changed: 45 additions & 14 deletions

File tree

apps/studio/forge.config.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,16 @@ const config: ForgeConfig = {
283283
// and @mistralai's ~200-char generated filenames, nested under pi-coding-agent, blow
284284
// past Windows' 260-char path limit and crash the Squirrel maker.
285285
console.log( 'Removing unused AI provider SDKs from CLI bundle...' );
286-
pruneUnusedProviders( cliNodeModules );
286+
const unprunedProviders = pruneUnusedProviders( cliNodeModules );
287+
if ( platform === 'win32' && unprunedProviders.length > 0 ) {
288+
// A leftover provider tree on Windows resurfaces as the PathTooLongException the
289+
// prune exists to prevent — fail now with the offending paths instead of letting
290+
// the Squirrel maker crash later with no context.
291+
throw new Error(
292+
`Could not prune ${ unprunedProviders.length } provider director(ies) that exceed ` +
293+
`Windows' 260-char path limit: ${ unprunedProviders.join( ', ' ) }`
294+
);
295+
}
287296

288297
console.log( `Downloading Node.js binary for ${ platform }-${ arch }...` );
289298
await execAsync( [

scripts/remove-unused-ai-providers.test.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs';
1+
import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs';
22
import { tmpdir } from 'os';
33
import { join } from 'path';
44
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
@@ -32,7 +32,7 @@ describe( 'pruneUnusedProviders', () => {
3232
touch( '@smithy', 'node-http-handler', 'package.json' );
3333
touch( '@google', 'genai', 'package.json' );
3434

35-
pruneUnusedProviders( nm );
35+
expect( pruneUnusedProviders( nm ) ).toEqual( [] );
3636

3737
expect( has( '@mistralai' ) ).toBe( false );
3838
expect( has( '@aws-sdk' ) ).toBe( false );
@@ -70,6 +70,19 @@ describe( 'pruneUnusedProviders', () => {
7070
} );
7171

7272
it( 'is a no-op when the node_modules directory is absent', () => {
73-
expect( () => pruneUnusedProviders( join( root, 'missing' ) ) ).not.toThrow();
73+
expect( pruneUnusedProviders( join( root, 'missing' ) ) ).toEqual( [] );
74+
} );
75+
76+
// A failed removal can't be induced portably; deletion permission lives on the parent
77+
// directory, which behaves differently on Windows and is ignored when running as root.
78+
const canInduceRemovalFailure = process.platform !== 'win32' && process.getuid?.() !== 0;
79+
it.runIf( canInduceRemovalFailure )( 'returns targets it could not remove', () => {
80+
touch( '@mistralai', 'mistralai', 'package.json' );
81+
chmodSync( nm, 0o555 ); // read-only parent → the child can't be unlinked
82+
try {
83+
expect( pruneUnusedProviders( nm ) ).toEqual( [ join( nm, '@mistralai' ) ] );
84+
} finally {
85+
chmodSync( nm, 0o755 ); // restore so afterEach can clean up
86+
}
7487
} );
7588
} );

scripts/remove-unused-ai-providers.ts

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,10 @@ const UNUSED_PROVIDER_DIRS = [
2929
join( '@google', 'genai' ),
3030
];
3131

32-
function removeTargets( nodeModulesDir: string ): void {
32+
// Returns the directories that should have been removed but couldn't, so the caller can decide
33+
// how to react (the Forge hook fails the Windows build on any leftover — see pruneUnusedProviders).
34+
function removeTargets( nodeModulesDir: string ): string[] {
35+
const failed: string[] = [];
3336
for ( const target of UNUSED_PROVIDER_DIRS ) {
3437
const dir = join( nodeModulesDir, target );
3538
try {
@@ -43,36 +46,40 @@ function removeTargets( nodeModulesDir: string ): void {
4346
rmSync( dir, { recursive: true, force: true } );
4447
console.log( `Removed ${ target } from ${ nodeModulesDir }` );
4548
} catch ( e ) {
46-
// Tolerate transient failures (Windows AV locks, permissions). This is a size /
47-
// path-length optimization; aborting packaging over a stuck file would be worse.
49+
// Don't abort here: on a transient lock (Windows AV, permissions) other targets may
50+
// still be removable, and the caller decides whether a leftover is fatal.
4851
console.warn(
4952
`Could not remove ${ target } from ${ nodeModulesDir }: ${
5053
e instanceof Error ? e.message : String( e )
5154
}`
5255
);
56+
failed.push( dir );
5357
}
5458
}
59+
return failed;
5560
}
5661

5762
/**
5863
* Walks the dependency tree and prunes the unused providers from every node_modules it finds —
59-
* the hoisted one and any nested under packages such as pi-coding-agent.
64+
* the hoisted one and any nested under packages such as pi-coding-agent. Returns the directories
65+
* that were targeted but couldn't be removed; on Windows a non-empty result is fatal, because a
66+
* leftover provider tree resurfaces as the very `PathTooLongException` this prune prevents.
6067
*/
61-
export function pruneUnusedProviders( nodeModulesDir: string ): void {
68+
export function pruneUnusedProviders( nodeModulesDir: string ): string[] {
6269
// Most packages have no nested node_modules; recursion bottoms out here. Check existence
6370
// up front so the common case is a cheap stat rather than a thrown-and-caught ENOENT.
6471
if ( ! existsSync( nodeModulesDir ) ) {
65-
return;
72+
return [];
6673
}
6774

6875
let entries;
6976
try {
7077
entries = readdirSync( nodeModulesDir, { withFileTypes: true } );
7178
} catch {
72-
return; // unreadable (permissions, races) — skip rather than abort packaging
79+
return []; // unreadable (permissions, races) — skip rather than abort packaging
7380
}
7481

75-
removeTargets( nodeModulesDir );
82+
const failed = removeTargets( nodeModulesDir );
7683

7784
for ( const entry of entries ) {
7885
if ( ! entry.isDirectory() ) {
@@ -89,11 +96,13 @@ export function pruneUnusedProviders( nodeModulesDir: string ): void {
8996
}
9097
for ( const scoped of scopedEntries ) {
9198
if ( scoped.isDirectory() ) {
92-
pruneUnusedProviders( join( packageDir, scoped.name, 'node_modules' ) );
99+
failed.push( ...pruneUnusedProviders( join( packageDir, scoped.name, 'node_modules' ) ) );
93100
}
94101
}
95102
continue;
96103
}
97-
pruneUnusedProviders( join( packageDir, 'node_modules' ) );
104+
failed.push( ...pruneUnusedProviders( join( packageDir, 'node_modules' ) ) );
98105
}
106+
107+
return failed;
99108
}

0 commit comments

Comments
 (0)