Skip to content

Commit 31981fa

Browse files
samuvsamuv
authored
fix(main): enhance PATH when spawning thv so macOS credential helpers resolve (#2100)
* refactor(main): extract createEnhancedPath helper into shared util * fix(main): enhance PATH when spawning thv so macOS credential helpers resolve * refactor(main): address PR feedback on enhanced-path helper
1 parent 2a15616 commit 31981fa

5 files changed

Lines changed: 210 additions & 39 deletions

File tree

‎main/src/container-engine.ts‎

Lines changed: 1 addition & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { exec } from 'node:child_process'
22
import { promisify } from 'node:util'
33
import { platform } from 'node:os'
44
import log from './logger'
5+
import { createEnhancedPath } from './utils/enhanced-path'
56

67
const execAsync = promisify(exec)
78

@@ -11,45 +12,6 @@ interface ContainerEngineStatus {
1112
available: boolean
1213
}
1314

14-
// Common installation paths by platform
15-
const getCommonPaths = (): string[] => {
16-
const currentPlatform = platform()
17-
18-
switch (currentPlatform) {
19-
case 'darwin':
20-
return [
21-
'/Applications/Docker.app/Contents/Resources/bin',
22-
'/opt/homebrew/bin',
23-
'/usr/local/bin',
24-
'~/.rd/bin',
25-
]
26-
case 'linux':
27-
return ['/usr/local/bin', '/opt/homebrew/bin', '/snap/bin', '~/.rd/bin']
28-
case 'win32':
29-
return [
30-
'C:\\Program Files\\Docker\\Docker\\resources\\bin',
31-
'C:\\Program Files\\RedHat\\Podman',
32-
]
33-
default:
34-
return []
35-
}
36-
}
37-
38-
const expandPath = (path: string): string =>
39-
path.startsWith('~')
40-
? path.replace('~', process.env.HOME || process.env.USERPROFILE || '')
41-
: path
42-
43-
const createEnhancedPath = (): string => {
44-
const commonPaths = getCommonPaths().map(expandPath)
45-
const currentPath = process.env.PATH || ''
46-
const separator = platform() === 'win32' ? ';' : ':'
47-
48-
return [...commonPaths, ...currentPath.split(separator)]
49-
.filter(Boolean)
50-
.join(separator)
51-
}
52-
5315
const tryCommand = async (command: string): Promise<boolean> => {
5416
try {
5517
await execAsync(command)

‎main/src/tests/toolhive-manager.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { EventEmitter } from 'node:events'
33
import { vol } from 'memfs'
44
import { spawn } from 'node:child_process'
55
import net from 'node:net'
6+
import { platform } from 'node:os'
67
import { app } from 'electron'
78
import {
89
startToolhive,
@@ -32,6 +33,18 @@ vi.mock('node:child_process', async (importOriginal) => {
3233
},
3334
}
3435
})
36+
vi.mock('node:os', async (importOriginal) => {
37+
const actual = await importOriginal<typeof import('node:os')>()
38+
const platformMock = vi.fn(actual.platform)
39+
return {
40+
...actual,
41+
platform: platformMock,
42+
default: {
43+
...actual,
44+
platform: platformMock,
45+
},
46+
}
47+
})
3548
vi.mock('node:fs')
3649
vi.mock('node:net', async (importOriginal) => {
3750
const actual = await importOriginal<typeof import('node:net')>()
@@ -454,6 +467,32 @@ describe('toolhive-manager', () => {
454467
expect(spawnArgs.some((a) => a.startsWith('--sentry-'))).toBe(false)
455468
})
456469

470+
it('spawns with an enhanced PATH that includes common container-tooling dirs', async () => {
471+
vi.mocked(platform).mockReturnValue('darwin')
472+
vi.stubEnv('PATH', '/usr/bin:/bin')
473+
474+
const startPromise = startToolhive()
475+
await vi.advanceTimersByTimeAsync(50)
476+
await startPromise
477+
478+
const spawnOptions = mockSpawn.mock.calls[0]![2] as {
479+
env: Record<string, string>
480+
}
481+
const spawnedPath = spawnOptions.env.PATH
482+
expect(spawnedPath).toBeDefined()
483+
const entries = spawnedPath!.split(':')
484+
485+
expect(entries).toContain(
486+
'/Applications/Docker.app/Contents/Resources/bin'
487+
)
488+
expect(entries).toContain('/opt/homebrew/bin')
489+
expect(entries).toContain('/usr/local/bin')
490+
expect(entries).toContain('/usr/bin')
491+
expect(entries).toContain('/bin')
492+
493+
expect(spawnOptions.env.TOOLHIVE_SKIP_DESKTOP_CHECK).toBe('true')
494+
})
495+
457496
it('omits sentry flags when telemetry is disabled', async () => {
458497
vi.stubEnv('VITE_SENTRY_THV_DSN', 'https://test@sentry.io/123')
459498
vi.mocked(readSetting).mockReturnValue('false')

‎main/src/toolhive-manager.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import log from './logger'
88
import * as Sentry from '@sentry/electron/main'
99
import { getQuittingState } from './app-state'
1010
import { readSetting } from './db/readers/settings-reader'
11+
import { createEnhancedPath } from './utils/enhanced-path'
1112
import {
1213
ALREADY_RUNNING,
1314
REGISTRY_AUTH_REQUIRED,
@@ -182,6 +183,7 @@ export async function startToolhive(): Promise<void> {
182183
windowsHide: true,
183184
env: {
184185
...process.env,
186+
PATH: createEnhancedPath(),
185187
TOOLHIVE_SKIP_DESKTOP_CHECK: 'true',
186188
},
187189
})

‎main/src/utils/__tests__/enhanced-path.test.ts‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'
2+
import { platform } from 'node:os'
3+
4+
import { createEnhancedPath } from '../enhanced-path'
5+
6+
vi.mock('node:os', async (importOriginal) => {
7+
const actual = await importOriginal<typeof import('node:os')>()
8+
const platformMock = vi.fn(actual.platform)
9+
return {
10+
...actual,
11+
platform: platformMock,
12+
default: {
13+
...actual,
14+
platform: platformMock,
15+
},
16+
}
17+
})
18+
19+
const mockPlatform = vi.mocked(platform)
20+
21+
describe('createEnhancedPath', () => {
22+
beforeEach(() => {
23+
mockPlatform.mockReset()
24+
})
25+
26+
afterEach(() => {
27+
vi.unstubAllEnvs()
28+
})
29+
30+
it('prepends macOS container-tooling paths on darwin', () => {
31+
mockPlatform.mockReturnValue('darwin')
32+
vi.stubEnv('PATH', '/usr/bin:/bin')
33+
vi.stubEnv('HOME', '/Users/test')
34+
35+
const result = createEnhancedPath()
36+
const entries = result.split(':')
37+
38+
expect(entries.slice(0, 4)).toEqual([
39+
'/Applications/Docker.app/Contents/Resources/bin',
40+
'/opt/homebrew/bin',
41+
'/usr/local/bin',
42+
'/Users/test/.rd/bin',
43+
])
44+
expect(entries).toContain('/usr/bin')
45+
expect(entries).toContain('/bin')
46+
})
47+
48+
it('expands ~ using HOME', () => {
49+
mockPlatform.mockReturnValue('darwin')
50+
vi.stubEnv('PATH', '')
51+
vi.stubEnv('HOME', '/Users/alice')
52+
53+
const result = createEnhancedPath()
54+
55+
expect(result).toContain('/Users/alice/.rd/bin')
56+
expect(result).not.toContain('~')
57+
})
58+
59+
it('keeps ~ verbatim when HOME and USERPROFILE are both unset', () => {
60+
mockPlatform.mockReturnValue('darwin')
61+
vi.stubEnv('PATH', '')
62+
vi.stubEnv('HOME', '')
63+
vi.stubEnv('USERPROFILE', '')
64+
65+
const result = createEnhancedPath()
66+
const entries = result.split(':')
67+
68+
expect(entries).toContain('~/.rd/bin')
69+
expect(entries).not.toContain('/.rd/bin')
70+
})
71+
72+
it('uses semicolon as separator on win32', () => {
73+
mockPlatform.mockReturnValue('win32')
74+
vi.stubEnv('PATH', 'C:\\Windows\\System32;C:\\Windows')
75+
76+
const result = createEnhancedPath()
77+
const entries = result.split(';')
78+
79+
expect(entries[0]).toBe('C:\\Program Files\\Docker\\Docker\\resources\\bin')
80+
expect(entries[1]).toBe('C:\\Program Files\\RedHat\\Podman')
81+
expect(entries).toContain('C:\\Windows\\System32')
82+
expect(entries).toContain('C:\\Windows')
83+
})
84+
85+
it('prepends linux container-tooling paths', () => {
86+
mockPlatform.mockReturnValue('linux')
87+
vi.stubEnv('PATH', '/usr/bin:/bin')
88+
vi.stubEnv('HOME', '/home/test')
89+
90+
const result = createEnhancedPath()
91+
const entries = result.split(':')
92+
93+
expect(entries.slice(0, 4)).toEqual([
94+
'/usr/local/bin',
95+
'/opt/homebrew/bin',
96+
'/snap/bin',
97+
'/home/test/.rd/bin',
98+
])
99+
})
100+
101+
it('preserves existing PATH entries after the prepended paths', () => {
102+
mockPlatform.mockReturnValue('darwin')
103+
vi.stubEnv('PATH', '/existing/bin:/another/bin')
104+
vi.stubEnv('HOME', '/Users/test')
105+
106+
const result = createEnhancedPath()
107+
const entries = result.split(':')
108+
109+
const existingIndex = entries.indexOf('/existing/bin')
110+
const anotherIndex = entries.indexOf('/another/bin')
111+
112+
expect(existingIndex).toBeGreaterThan(-1)
113+
expect(anotherIndex).toBeGreaterThan(existingIndex)
114+
})
115+
116+
it('handles empty PATH gracefully', () => {
117+
mockPlatform.mockReturnValue('darwin')
118+
vi.stubEnv('PATH', '')
119+
vi.stubEnv('HOME', '/Users/test')
120+
121+
const result = createEnhancedPath()
122+
123+
expect(result).not.toBe('')
124+
expect(result).toContain('/usr/local/bin')
125+
// No trailing empty segments from the split/join
126+
expect(result.endsWith(':')).toBe(false)
127+
})
128+
})

‎main/src/utils/enhanced-path.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { platform } from 'node:os'
2+
3+
const getCommonPaths = (): string[] => {
4+
const currentPlatform = platform()
5+
6+
switch (currentPlatform) {
7+
case 'darwin':
8+
return [
9+
'/Applications/Docker.app/Contents/Resources/bin',
10+
'/opt/homebrew/bin',
11+
'/usr/local/bin',
12+
'~/.rd/bin',
13+
]
14+
case 'linux':
15+
return ['/usr/local/bin', '/opt/homebrew/bin', '/snap/bin', '~/.rd/bin']
16+
case 'win32':
17+
return [
18+
'C:\\Program Files\\Docker\\Docker\\resources\\bin',
19+
'C:\\Program Files\\RedHat\\Podman',
20+
]
21+
default:
22+
return []
23+
}
24+
}
25+
26+
const expandPath = (path: string): string => {
27+
if (!path.startsWith('~')) return path
28+
const homeDir = process.env.HOME || process.env.USERPROFILE
29+
return homeDir ? path.replace('~', homeDir) : path
30+
}
31+
32+
export const createEnhancedPath = (): string => {
33+
const commonPaths = getCommonPaths().map(expandPath)
34+
const currentPath = process.env.PATH || ''
35+
const separator = platform() === 'win32' ? ';' : ':'
36+
37+
return [...commonPaths, ...currentPath.split(separator)]
38+
.filter(Boolean)
39+
.join(separator)
40+
}

0 commit comments

Comments
 (0)