mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 16:03:20 +00:00
fix(windows): follow the host block's PATH spelling on sparse daemon env patches
Resolving a path-less Windows env to `Path` handed the daemon's own
`{...process.env, ...opts.env}` merge both spellings when the host block spelt
`PATH`. Fall back to the host block's own key, and collapse again inside the
daemon since that merge happens after attribution.
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -83,6 +83,41 @@ describe('applyTerminalAttributionEnv PATH key casing', () => {
|
||||
expect(baseEnv.Path).toBe('C:\\Windows\\system32')
|
||||
})
|
||||
|
||||
// Why: the daemon receives a sparse env patch with no path key and re-merges its own block
|
||||
// underneath it, so guessing the spelling here hands the child both.
|
||||
it.each([
|
||||
{ hostKey: 'PATH', otherKey: 'Path' },
|
||||
{ hostKey: 'Path', otherKey: 'PATH' }
|
||||
])('adopts a host block spelt `$hostKey` on a path-less env', ({ hostKey, otherKey }) => {
|
||||
const root = makeTmpRoot()
|
||||
const userDataPath = join(root, 'user-data')
|
||||
const inherited = { PATH: process.env.PATH, Path: process.env.Path }
|
||||
delete process.env.PATH
|
||||
delete process.env.Path
|
||||
process.env[hostKey] = 'C:\\Windows\\system32'
|
||||
|
||||
try {
|
||||
const baseEnv: Record<string, string> = { SystemRoot: 'C:\\Windows' }
|
||||
|
||||
applyTerminalAttributionEnv(baseEnv, {
|
||||
enabled: true,
|
||||
platform: 'win32',
|
||||
shellFamily: 'native-windows',
|
||||
userDataPath
|
||||
})
|
||||
|
||||
expect(pathEnvKeys(baseEnv)).toEqual([hostKey])
|
||||
expect(baseEnv[otherKey]).toBeUndefined()
|
||||
} finally {
|
||||
delete process.env[hostKey]
|
||||
for (const [key, value] of Object.entries(inherited)) {
|
||||
if (value !== undefined) {
|
||||
process.env[key] = value
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
it('leaves a case-sensitive POSIX `Path` variable alone', () => {
|
||||
const root = makeTmpRoot()
|
||||
const baseEnv: Record<string, string> = {
|
||||
|
||||
@@ -147,9 +147,9 @@ function clearTerminalAttributionEnv(
|
||||
/**
|
||||
* Drops the non-resolved PATH spelling so a Windows child inherits exactly one.
|
||||
*
|
||||
* Why: attribution is the last env-key-shaping step before spawn, making it the single
|
||||
* choke point where an inherited duplicate (WSL interop, third-party tools) can be
|
||||
* collapsed deterministically instead of leaving the OS to pick a winner.
|
||||
* Why: an inherited duplicate (WSL interop, third-party tools) is collapsed here rather than
|
||||
* left for the OS to pick a winner. Providers that re-merge their own block after this point
|
||||
* (the daemon) collapse again on their side, since this cannot see that merge.
|
||||
*/
|
||||
function collapseWindowsPathEnvKey(
|
||||
baseEnv: Record<string, string>,
|
||||
|
||||
@@ -2113,7 +2113,7 @@ describe('createPtySubprocess', () => {
|
||||
expect(lastCall[2].env.ORCA_ATTRIBUTION_SHIM_DIR).toBeUndefined()
|
||||
})
|
||||
|
||||
it('promotes the agent-teams shim onto the Windows `Path` spelling', () => {
|
||||
it('collapses its own env merge onto the requested Windows `Path` spelling', () => {
|
||||
const proc = mockPtyProcess()
|
||||
spawnMock.mockReturnValue(proc)
|
||||
const platform = Object.getOwnPropertyDescriptor(process, 'platform')
|
||||
@@ -2124,7 +2124,8 @@ describe('createPtySubprocess', () => {
|
||||
sessionId: 'test',
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
// Why: buildPtyHostEnv collapses Windows PATH onto one spelling before the daemon wire.
|
||||
// Why: buildPtyHostEnv collapses Windows PATH onto one spelling before the daemon wire;
|
||||
// the daemon then spreads its own block underneath and can re-mint the other one.
|
||||
env: {
|
||||
Path: '/tmp/orca-agent-teams-bin:/usr/bin',
|
||||
ORCA_AGENT_TEAMS_TEAM_ID: 'team-test'
|
||||
@@ -2137,7 +2138,25 @@ describe('createPtySubprocess', () => {
|
||||
}
|
||||
|
||||
const env = spawnMock.mock.calls.at(-1)![2].env
|
||||
expect(Object.keys(env).filter((key) => /^path$/i.test(key))).toEqual(['Path'])
|
||||
expect(env.Path.split(':')[0]).toBe('/tmp/orca-agent-teams-bin')
|
||||
})
|
||||
|
||||
it('keeps the daemon `PATH` block when the requested env has no path key', () => {
|
||||
const proc = mockPtyProcess()
|
||||
spawnMock.mockReturnValue(proc)
|
||||
const platform = Object.getOwnPropertyDescriptor(process, 'platform')
|
||||
|
||||
Object.defineProperty(process, 'platform', { value: 'win32' })
|
||||
try {
|
||||
createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24, env: { FOO: 'bar' } })
|
||||
} finally {
|
||||
if (platform) {
|
||||
Object.defineProperty(process, 'platform', platform)
|
||||
}
|
||||
}
|
||||
|
||||
const env = spawnMock.mock.calls.at(-1)![2].env
|
||||
expect(env.PATH).toBe(process.env.PATH)
|
||||
})
|
||||
|
||||
|
||||
@@ -74,6 +74,7 @@ const PANE_IDENTITY_ENV_KEYS = [
|
||||
'ORCA_WORKTREE_ID',
|
||||
'ORCA_AGENT_LAUNCH_TOKEN'
|
||||
] as const
|
||||
const WINDOWS_PATH_ENV_KEY_RE = /^path$/i
|
||||
const FOREGROUND_AGENT_CACHE_TTL_MS = 1000
|
||||
const SHELL_FOREGROUND_REFRESH_RETRY_MS = 5_000
|
||||
// Why: a Windows refresh forks a heavy powershell.exe CIM scan (~10-40x POSIX `ps`); idle shells retry slower, output re-arms the fast retry.
|
||||
@@ -162,6 +163,36 @@ function removeUnspecifiedPaneIdentityEnv(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Collapses the duplicate Windows path spelling this daemon's own env merge can mint.
|
||||
*
|
||||
* Why: main sends a sparse env patch, so `{...process.env, ...opts.env}` can pair the daemon
|
||||
* block's spelling with the patch's; a Windows child inheriting both crashes the packaged CLI
|
||||
* launcher with "Item has already been added. Key in dictionary: 'PATH'" (stablyai/orca#12046).
|
||||
*/
|
||||
function collapseWindowsPathEnvKeys(
|
||||
env: Record<string, string>,
|
||||
requestedEnv: Record<string, string> | undefined
|
||||
): void {
|
||||
if (process.platform !== 'win32') {
|
||||
return
|
||||
}
|
||||
const pathKeys = Object.keys(env).filter((key) => WINDOWS_PATH_ENV_KEY_RE.test(key))
|
||||
if (pathKeys.length < 2) {
|
||||
return
|
||||
}
|
||||
// Why: main already shaped the patch's spelling (attribution, CLI shims), so its value wins.
|
||||
const requestedKey = requestedEnv
|
||||
? Object.keys(requestedEnv).find((key) => WINDOWS_PATH_ENV_KEY_RE.test(key))
|
||||
: undefined
|
||||
const survivingKey = requestedKey && env[requestedKey] !== undefined ? requestedKey : pathKeys[0]
|
||||
for (const key of pathKeys) {
|
||||
if (key !== survivingKey) {
|
||||
delete env[key]
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Promotes the agent-teams shim path ahead of inherited PATH entries.
|
||||
*/
|
||||
@@ -770,8 +801,9 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl
|
||||
) {
|
||||
addWslEnvKeys(env, [POWERLEVEL10K_WIZARD_DISABLE_ENV])
|
||||
}
|
||||
// Why: opts.env arrives from buildPtyHostEnv, which collapses Windows PATH onto a single spelling.
|
||||
const requestedEnv = opts.env
|
||||
// Why: collapse before promoting so the shim lands on the spelling the child actually inherits.
|
||||
collapseWindowsPathEnvKeys(env, requestedEnv)
|
||||
const requestedPath = requestedEnv
|
||||
? requestedEnv[resolvePathEnvKey(requestedEnv, process.platform)]
|
||||
: undefined
|
||||
|
||||
@@ -310,8 +310,11 @@ describe('resolvePathEnvKey', () => {
|
||||
expect(resolvePathEnvKey({ PATH: 'C:\\Windows' }, 'win32')).toBe('PATH')
|
||||
})
|
||||
|
||||
it('defaults to the OS spelling for a Windows env with no path key', () => {
|
||||
expect(resolvePathEnvKey({}, 'win32')).toBe('Path')
|
||||
it('falls back to the host block spelling for a Windows env with no path key', () => {
|
||||
// Why: a sparse patch that guesses wrong leaves the daemon's own merge holding both keys.
|
||||
expect(resolvePathEnvKey({}, 'win32', { PATH: 'C:\\Windows' })).toBe('PATH')
|
||||
expect(resolvePathEnvKey({}, 'win32', { Path: 'C:\\Windows' })).toBe('Path')
|
||||
expect(resolvePathEnvKey({}, 'win32', {})).toBe('Path')
|
||||
})
|
||||
|
||||
it('always resolves `PATH` off Windows so a POSIX `Path` variable is untouched', () => {
|
||||
|
||||
@@ -249,12 +249,31 @@ export function __resetPersistedWindowsPathCacheForTests(): void {
|
||||
*/
|
||||
export function resolvePathEnvKey(
|
||||
env: NodeJS.ProcessEnv | Record<string, string | undefined>,
|
||||
platform: NodeJS.Platform
|
||||
platform: NodeJS.Platform,
|
||||
hostEnv: NodeJS.ProcessEnv = process.env
|
||||
): 'PATH' | 'Path' {
|
||||
if (platform !== 'win32') {
|
||||
return 'PATH'
|
||||
}
|
||||
return env.Path !== undefined ? 'Path' : env.PATH !== undefined ? 'PATH' : 'Path'
|
||||
if (env.Path !== undefined) {
|
||||
return 'Path'
|
||||
}
|
||||
if (env.PATH !== undefined) {
|
||||
return 'PATH'
|
||||
}
|
||||
// Why: the daemon receives a sparse env patch and re-merges its own block underneath it;
|
||||
// matching the block's spelling stops that merge from resurrecting the other one.
|
||||
return readHostPathEnvKey(hostEnv)
|
||||
}
|
||||
|
||||
/**
|
||||
* Reads the path spelling the host's own environment block carries.
|
||||
*
|
||||
* Why: Node's Windows `process.env` lookups are case-insensitive, so only the own keys
|
||||
* reveal which spelling a child process will actually inherit.
|
||||
*/
|
||||
function readHostPathEnvKey(hostEnv: NodeJS.ProcessEnv): 'PATH' | 'Path' {
|
||||
return Object.keys(hostEnv).includes('PATH') ? 'PATH' : 'Path'
|
||||
}
|
||||
|
||||
function mergeWindowsPathSegments(
|
||||
@@ -263,7 +282,7 @@ function mergeWindowsPathSegments(
|
||||
platform: NodeJS.Platform,
|
||||
sourceEnv: NodeJS.ProcessEnv
|
||||
): void {
|
||||
const pathKey = resolvePathEnvKey(env, platform)
|
||||
const pathKey = resolvePathEnvKey(env, platform, sourceEnv)
|
||||
const pathDelimiter = getPathDelimiter(platform)
|
||||
const currentPath = env[pathKey] ?? sourceEnv.PATH ?? sourceEnv.Path ?? ''
|
||||
const currentSegments = splitPathSegments(currentPath, pathDelimiter)
|
||||
|
||||
Reference in New Issue
Block a user