Skip to content

Commit 245c1e8

Browse files
jdaltonjdalton
authored
Fix shadow bin argument passing causing findLast errors (#912)
* Fix shadow bin argument passing - args were shifted incorrectly The shadow-bin/npm and shadow-bin/npx files were incorrectly passing the bin name ('npm'/'npx') as the first argument, which caused args to be set to a string instead of an array. This resulted in 'findLast is not a function' errors because strings don't have the findLast method. - shadow-bin/npm: Remove 'npm' argument (already hardcoded in shadowNpmBin) - shadow-bin/npx: Use correct shadowNpxBin function and remove 'npx' argument Fixes #911 * Add unit tests for shadow npm and npx bin functions Tests validate that args parameter is correctly passed as an array and that Array.prototype.findLast() works properly. These tests prevent regression of issue #911 where incorrect argument passing caused args to be a string instead of an array. Tests cover: - Array argument handling - Readonly array support - Empty array handling - Terminator (--) handling - findLast method availability - Progress flag identification using findLast
1 parent afbe90a commit 245c1e8

File tree

4 files changed

+257
-3
lines changed

4 files changed

+257
-3
lines changed

‎shadow-bin/npm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ void (async () => {
1111

1212
process.exitCode = 1
1313

14-
const { spawnPromise } = await shadowNpmBin('npm', process.argv.slice(2), { stdio: 'inherit' })
14+
const { spawnPromise } = await shadowNpmBin(process.argv.slice(2), { stdio: 'inherit' })
1515

1616
// See https://nodejs.org/api/child_process.html#event-exit.
1717
spawnPromise.process.on('exit', (code, signalName) => {

‎shadow-bin/npx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ void (async () => {
77
const rootPath = path.join(__dirname, '..')
88
Module.enableCompileCache?.(path.join(rootPath, '.cache'))
99

10-
const shadowNpmBin = require(path.join(rootPath, 'dist/shadow-npm-bin.js'))
10+
const shadowNpxBin = require(path.join(rootPath, 'dist/shadow-npx-bin.js'))
1111

1212
process.exitCode = 1
1313

14-
const { spawnPromise } = await shadowNpmBin('npx', process.argv.slice(2), { stdio: 'inherit' })
14+
const { spawnPromise } = await shadowNpxBin(process.argv.slice(2), { stdio: 'inherit' })
1515

1616
// See https://nodejs.org/api/child_process.html#event-exit.
1717
spawnPromise.process.on('exit', (code, signalName) => {

‎src/shadow/npm/bin.test.mts‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
import shadowNpmBin from './bin.mts'
4+
5+
// Mock all dependencies with vi.hoisted for better type safety.
6+
const mockInstallNpmLinks = vi.hoisted(() => vi.fn())
7+
const mockSpawn = vi.hoisted(() => vi.fn())
8+
const mockFindUp = vi.hoisted(() => vi.fn())
9+
10+
vi.mock('../../utils/shadow-links.mts', () => ({
11+
installNpmLinks: mockInstallNpmLinks,
12+
}))
13+
14+
vi.mock('@socketsecurity/registry/lib/spawn', () => ({
15+
spawn: mockSpawn,
16+
}))
17+
18+
vi.mock('../../utils/fs.mts', () => ({
19+
findUp: mockFindUp,
20+
}))
21+
22+
vi.mock('../../constants.mts', async importOriginal => {
23+
const actual = (await importOriginal()) as Record<string, any>
24+
return {
25+
...actual,
26+
default: {
27+
...actual?.default,
28+
shadowBinPath: '/mock/shadow-bin',
29+
shadowNpmInjectPath: '/mock/inject.js',
30+
execPath: '/usr/bin/node',
31+
npmGlobalPrefix: '/usr/local',
32+
npmCachePath: '/home/.npm',
33+
nodeNoWarningsFlags: [],
34+
nodeDebugFlags: [],
35+
nodeHardenFlags: [],
36+
nodeMemoryFlags: [],
37+
processEnv: {},
38+
ENV: {
39+
INLINED_SOCKET_CLI_SENTRY_BUILD: false,
40+
},
41+
SUPPORTS_NODE_PERMISSION_FLAG: false,
42+
},
43+
}
44+
})
45+
46+
describe('shadowNpmBin', () => {
47+
beforeEach(() => {
48+
vi.clearAllMocks()
49+
50+
// Default mock implementations.
51+
mockInstallNpmLinks.mockResolvedValue('/usr/bin/npm')
52+
mockFindUp.mockResolvedValue(null)
53+
mockSpawn.mockReturnValue({
54+
process: {
55+
send: vi.fn(),
56+
on: vi.fn(),
57+
},
58+
then: vi.fn().mockImplementation(cb =>
59+
cb({
60+
success: true,
61+
code: 0,
62+
stdout: '',
63+
stderr: '',
64+
}),
65+
),
66+
})
67+
})
68+
69+
it('should accept args as an array and handle findLast correctly', async () => {
70+
const args = ['install', '--no-progress', '--loglevel=error']
71+
const result = await shadowNpmBin(args)
72+
73+
expect(result).toHaveProperty('spawnPromise')
74+
expect(mockSpawn).toHaveBeenCalled()
75+
76+
// Verify spawn was called with correct arguments.
77+
const spawnArgs = mockSpawn.mock.calls[0]
78+
expect(spawnArgs).toBeDefined()
79+
})
80+
81+
it('should handle array with terminator correctly', async () => {
82+
const args = ['install', '--no-progress', '--', 'extra', 'args']
83+
const result = await shadowNpmBin(args)
84+
85+
expect(result).toHaveProperty('spawnPromise')
86+
expect(mockSpawn).toHaveBeenCalled()
87+
})
88+
89+
it('should handle empty args array', async () => {
90+
const args: string[] = []
91+
const result = await shadowNpmBin(args)
92+
93+
expect(result).toHaveProperty('spawnPromise')
94+
expect(mockSpawn).toHaveBeenCalled()
95+
})
96+
97+
it('should handle readonly array correctly', async () => {
98+
const args: readonly string[] = ['install', '--no-progress'] as const
99+
const result = await shadowNpmBin(args)
100+
101+
expect(result).toHaveProperty('spawnPromise')
102+
expect(mockSpawn).toHaveBeenCalled()
103+
})
104+
105+
it('should not throw "findLast is not a function" error', async () => {
106+
// This test specifically validates the fix for issue #911.
107+
// The bug was caused by passing a string instead of an array,
108+
// which made rawBinArgs.findLast() fail because strings don't
109+
// have the findLast method.
110+
const args = ['install', '--progress']
111+
112+
await expect(shadowNpmBin(args)).resolves.toHaveProperty('spawnPromise')
113+
})
114+
115+
it('should correctly identify progress flags using findLast', async () => {
116+
// Test that findLast correctly finds the last progress flag.
117+
const args = ['install', '--progress', '--no-progress']
118+
await shadowNpmBin(args)
119+
120+
// Verify spawn was called - the actual flag processing happens inside.
121+
expect(mockSpawn).toHaveBeenCalled()
122+
const spawnArgs = mockSpawn.mock.calls[0][1] as string[]
123+
124+
// Should include --no-progress in the final args since it was last.
125+
expect(spawnArgs).toContain('--no-progress')
126+
})
127+
})

‎src/shadow/npx/bin.test.mts‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
import shadowNpxBin from './bin.mts'
4+
5+
// Mock all dependencies with vi.hoisted for better type safety.
6+
const mockInstallNpxLinks = vi.hoisted(() => vi.fn())
7+
const mockSpawn = vi.hoisted(() => vi.fn())
8+
const mockFindUp = vi.hoisted(() => vi.fn())
9+
10+
vi.mock('../../utils/shadow-links.mts', () => ({
11+
installNpxLinks: mockInstallNpxLinks,
12+
}))
13+
14+
vi.mock('@socketsecurity/registry/lib/spawn', () => ({
15+
spawn: mockSpawn,
16+
}))
17+
18+
vi.mock('../../utils/fs.mts', () => ({
19+
findUp: mockFindUp,
20+
}))
21+
22+
vi.mock('../../constants.mts', async importOriginal => {
23+
const actual = (await importOriginal()) as Record<string, any>
24+
return {
25+
...actual,
26+
default: {
27+
...actual?.default,
28+
shadowBinPath: '/mock/shadow-bin',
29+
shadowNpmInjectPath: '/mock/inject.js',
30+
execPath: '/usr/bin/node',
31+
npmGlobalPrefix: '/usr/local',
32+
npmCachePath: '/usr/local/.npm',
33+
nodeNoWarningsFlags: [],
34+
nodeDebugFlags: [],
35+
nodeHardenFlags: [],
36+
nodeMemoryFlags: [],
37+
processEnv: {},
38+
ENV: {
39+
INLINED_SOCKET_CLI_SENTRY_BUILD: false,
40+
},
41+
SUPPORTS_NODE_PERMISSION_FLAG: false,
42+
},
43+
}
44+
})
45+
46+
describe('shadowNpxBin', () => {
47+
beforeEach(() => {
48+
vi.clearAllMocks()
49+
50+
// Default mock implementations.
51+
mockInstallNpxLinks.mockResolvedValue('/usr/bin/npx')
52+
mockFindUp.mockResolvedValue(null)
53+
mockSpawn.mockReturnValue({
54+
process: {
55+
send: vi.fn(),
56+
on: vi.fn(),
57+
},
58+
then: vi.fn().mockImplementation(cb =>
59+
cb({
60+
success: true,
61+
code: 0,
62+
stdout: '',
63+
stderr: '',
64+
}),
65+
),
66+
})
67+
})
68+
69+
it('should accept args as an array and handle findLast correctly', async () => {
70+
const args = ['cowsay', 'hello', '--no-progress']
71+
const result = await shadowNpxBin(args)
72+
73+
expect(result).toHaveProperty('spawnPromise')
74+
expect(mockSpawn).toHaveBeenCalled()
75+
76+
// Verify spawn was called with correct arguments.
77+
const spawnArgs = mockSpawn.mock.calls[0]
78+
expect(spawnArgs).toBeDefined()
79+
})
80+
81+
it('should handle array with terminator correctly', async () => {
82+
const args = ['cowsay', '--', 'extra', 'args']
83+
const result = await shadowNpxBin(args)
84+
85+
expect(result).toHaveProperty('spawnPromise')
86+
expect(mockSpawn).toHaveBeenCalled()
87+
})
88+
89+
it('should handle empty args array', async () => {
90+
const args: string[] = []
91+
const result = await shadowNpxBin(args)
92+
93+
expect(result).toHaveProperty('spawnPromise')
94+
expect(mockSpawn).toHaveBeenCalled()
95+
})
96+
97+
it('should handle readonly array correctly', async () => {
98+
const args: readonly string[] = ['cowsay', 'hello'] as const
99+
const result = await shadowNpxBin(args)
100+
101+
expect(result).toHaveProperty('spawnPromise')
102+
expect(mockSpawn).toHaveBeenCalled()
103+
})
104+
105+
it('should not throw "findLast is not a function" error', async () => {
106+
// This test specifically validates the fix for issue #911.
107+
// The bug was caused by passing a string instead of an array,
108+
// which made rawBinArgs.findLast() fail because strings don't
109+
// have the findLast method.
110+
const args = ['cowsay', '--progress']
111+
112+
await expect(shadowNpxBin(args)).resolves.toHaveProperty('spawnPromise')
113+
})
114+
115+
it('should correctly identify progress flags using findLast', async () => {
116+
// Test that findLast correctly finds the last progress flag.
117+
const args = ['cowsay', '--progress', '--no-progress']
118+
await shadowNpxBin(args)
119+
120+
// Verify spawn was called - the actual flag processing happens inside.
121+
expect(mockSpawn).toHaveBeenCalled()
122+
const spawnArgs = mockSpawn.mock.calls[0][1] as string[]
123+
124+
// Should include --no-progress in the final args since it was last.
125+
expect(spawnArgs).toContain('--no-progress')
126+
})
127+
})

0 commit comments

Comments
 (0)