mirror of
https://github.com/stablyai/orca.git
synced 2026-09-23 00:02:29 +00:00
* fix(cli): seed nvm's default version, not the newest install #16314 stopped the login-shell probe inheriting the seeded PATH, but left the seed itself picking the newest installed nvm version. That ordering decides which node a CLI runs under whenever the probe does not land — a timeout, or a login shell whose rc never initializes nvm — and newest is precisely the wrong guess: it is usually the version the user just added and has installed nothing into. That is the root cause reported in #10932. Resolve `alias/default` instead, mirroring nvm: follow the alias chain (`default` -> `lts/*` -> `lts/krypton` -> a version), resolve a partial version like `24` to the highest matching install, and treat `system`/`node`/`stable` as no preference. The chain is bounded and cycle-guarded because nvm's own resolver tracks seen aliases and hand-edited files can point at each other. Ordering is a preference, not a restriction: the remaining versions stay behind the default, so a CLI installed outside it is still reachable. Measured on a real machine with nvm default=24 and a bare v26.7.0 installed: the old resolver seeds v26.7.0/bin (no CLIs), the new one seeds v24.18.0/bin (every CLI). Tests were written first and verified to fail on the three bug cases against main before the fix existed. Also raise the probe budget from 5s to 10s. The old value was never measured against a real profile: a bash -ilc loading nvm, rvm, conda and gcloud takes ~1s idle but 6-7s on a loaded machine, so a cold start under load silently fell back to the seed. Startup does not block on the probe, and the one awaited consumer is agent detection, which is better served by a probe that finishes late than one that gives up early. * fix(cli): reject non-version alias tokens instead of matching v0.x Review finding, and a real bug I introduced. parseVersionSegment coerces every unparseable segment to 0, so an unresolvable default alias — `garbage`, `iojs`, `lts/nonexistent`, any hand-named alias — became [0] and prefix-matched a `v0.12.x` install, or any stray non-version directory. Orca would then seed a decade-old node as the preferred runtime. Real nvm answers N/A for all of them. The `wanted.length === 0` bail could never have caught this: ''.split('.') is [''], never empty. Replaced with a shape check that still admits legitimate numeric prefixes — verified against nvm itself, which resolves `24` to v24.18.0 and `0` to an installed v0.x while answering N/A for the rest. Also corrects two comments that no longer described the code: the seed is no longer "newest install", and the probe budget note claimed startup never blocks on hydration, which is false on packaged Windows where it gates terminal services and git. The traversal-guard comment claimed a containment join() already normalizes away; the real guarantee is that matchNvmVersion can only return an entry of the versions directory. * fix(cli): match nvm's version-token grammar, not just its first character Round-2 review finding, and the same bug one layer down. The previous guard anchored only the first character, but parseInt stops at the first non-digit, so `0x18`, `00` and `0abc` still parsed to [0] and prefix-matched a v0.12.x install — the decade-old-node seed the earlier fix was supposed to close. Reachable: `nvm alias default 0x18` warns that the version does not exist and writes the alias anyway, then resolves it to N/A. Use nvm's actual grammar, leading zeros included — nvm calls `00` and `024` N/A while parseInt reads them as 0 and 24. Verified by executing 17 tokens against a five-version fixture: every one now agrees with nvm, including the legitimate prefixes `0`, `0.12`, `24` and `v24.18.0`. Also drops a dead disjunct (the hop bound already caps the loop, so seen.size can never exceed it) and corrects the log comment in index.ts, which still told the reader a failed probe leaves the newest install in front. It leaves the default version in front now, which is usually survivable but still not what the shell would have resolved. * test(cli): skip the lts/* chain fixture on Windows Round-3 review finding. makeNvmHome materializes each alias as a real file, and the chain case uses nvm's actual `lts/*` alias — `*` is a reserved Win32 filename character, so writeFileSync fails with EINVAL. PR CI runs a Windows allowlist that excludes this file, so the breakage only reaches a Windows developer running the suite locally. Skipped rather than renamed: `lts/*` is the alias nvm really ships, and the assertion pins platform: 'darwin' anyway, so the real name costs no coverage. Matches the skipIf convention already used across src/shared. Also reflows a comment line that a previous edit ran to 143 characters; oxfmt does not reflow comments, so nothing would have caught it.
159 lines
6.3 KiB
TypeScript
159 lines
6.3 KiB
TypeScript
import { chmodSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'
|
|
import { tmpdir } from 'node:os'
|
|
import { join } from 'node:path'
|
|
import { describe, expect, it } from 'vitest'
|
|
import { getVersionManagerBinPaths, resolveCliCommand } from './node-cli-command-resolution'
|
|
|
|
/**
|
|
* The seed decides which node a CLI runs under whenever the login-shell probe
|
|
* does not land (timeout, or a shell whose rc never initializes nvm). Picking
|
|
* the newest install there is what broke #10932: the newest version is usually
|
|
* the one the user just added and has installed nothing into.
|
|
*/
|
|
function makeNvmHome(options: {
|
|
versions: string[]
|
|
defaultAlias?: string
|
|
aliases?: Record<string, string>
|
|
cliIn?: string
|
|
}): string {
|
|
const home = mkdtempSync(join(tmpdir(), 'orca-nvm-'))
|
|
for (const version of options.versions) {
|
|
const bin = join(home, '.nvm', 'versions', 'node', version, 'bin')
|
|
mkdirSync(bin, { recursive: true })
|
|
for (const name of ['node', 'node.exe']) {
|
|
writeFileSync(join(bin, name), '')
|
|
chmodSync(join(bin, name), 0o755)
|
|
}
|
|
if (options.cliIn === version) {
|
|
writeFileSync(join(bin, 'codex'), '')
|
|
chmodSync(join(bin, 'codex'), 0o755)
|
|
}
|
|
}
|
|
if (options.defaultAlias !== undefined) {
|
|
mkdirSync(join(home, '.nvm', 'alias'), { recursive: true })
|
|
writeFileSync(join(home, '.nvm', 'alias', 'default'), `${options.defaultAlias}\n`)
|
|
}
|
|
for (const [name, value] of Object.entries(options.aliases ?? {})) {
|
|
const file = join(home, '.nvm', 'alias', name)
|
|
mkdirSync(join(file, '..'), { recursive: true })
|
|
writeFileSync(file, `${value}\n`)
|
|
}
|
|
return home
|
|
}
|
|
|
|
function seededNvmDir(homePath: string): string | undefined {
|
|
return getVersionManagerBinPaths({ platform: 'darwin', pathEnv: '', homePath }).find((entry) =>
|
|
entry.includes('.nvm')
|
|
)
|
|
}
|
|
|
|
describe('nvm default alias decides the seeded runtime', () => {
|
|
it('seeds the default version, not the newest install (#10932)', () => {
|
|
// The exact shape that broke: `nvm install 26` adds a bare newest version
|
|
// while every CLI lives under the default.
|
|
const home = makeNvmHome({
|
|
versions: ['v24.18.0', 'v26.7.0'],
|
|
defaultAlias: '24',
|
|
cliIn: 'v24.18.0'
|
|
})
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v24.18.0', 'bin'))
|
|
})
|
|
|
|
it('resolves a partial default to the highest matching install', () => {
|
|
const home = makeNvmHome({
|
|
versions: ['v24.9.0', 'v24.18.0', 'v26.7.0'],
|
|
defaultAlias: '24',
|
|
cliIn: 'v24.18.0'
|
|
})
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v24.18.0', 'bin'))
|
|
})
|
|
|
|
// Why skipIf rather than renaming the fixture: `lts/*` is the real alias nvm
|
|
// ships, and `*` is a reserved Win32 filename character, so the fixture cannot
|
|
// be materialized there. Keeping the real name is worth more than the case
|
|
// running on a platform where seededNvmDir already pins platform: 'darwin'.
|
|
it.skipIf(process.platform === 'win32')(
|
|
'follows an alias chain (default -> lts/* -> lts/krypton -> version)',
|
|
() => {
|
|
const home = makeNvmHome({
|
|
versions: ['v22.9.0', 'v26.7.0'],
|
|
defaultAlias: 'lts/*',
|
|
aliases: { 'lts/*': 'lts/krypton', 'lts/krypton': 'v22.9.0' },
|
|
cliIn: 'v22.9.0'
|
|
})
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v22.9.0', 'bin'))
|
|
}
|
|
)
|
|
|
|
it('falls back to newest when there is no default alias', () => {
|
|
const home = makeNvmHome({ versions: ['v24.18.0', 'v26.7.0'] })
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v26.7.0', 'bin'))
|
|
})
|
|
|
|
it('falls back to newest when the default names an uninstalled version', () => {
|
|
const home = makeNvmHome({ versions: ['v24.18.0', 'v26.7.0'], defaultAlias: '18' })
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v26.7.0', 'bin'))
|
|
})
|
|
|
|
it('survives a cyclic alias chain instead of hanging', () => {
|
|
const home = makeNvmHome({
|
|
versions: ['v24.18.0', 'v26.7.0'],
|
|
defaultAlias: 'a',
|
|
aliases: { a: 'b', b: 'a' }
|
|
})
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v26.7.0', 'bin'))
|
|
})
|
|
|
|
it('ignores a `system` default, which means no nvm node at all', () => {
|
|
const home = makeNvmHome({ versions: ['v24.18.0', 'v26.7.0'], defaultAlias: 'system' })
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v26.7.0', 'bin'))
|
|
})
|
|
|
|
// Why the digit-leading entries: parseInt stops at the first non-digit, so
|
|
// anchoring only the first character still let `0x18` and `00` parse to 0 and
|
|
// prefix-match a decade-old v0.x. nvm writes such a token to the alias file
|
|
// even while warning it does not exist, then answers N/A for it.
|
|
it.each([
|
|
'garbage',
|
|
'iojs',
|
|
'lts/nonexistent',
|
|
'my-custom-alias',
|
|
'0x18',
|
|
'00',
|
|
'0abc',
|
|
'024',
|
|
'24abc',
|
|
'v24.18.0-nightly',
|
|
'V24'
|
|
])('treats the unresolvable default %s as no preference, like nvm N/A', (token) => {
|
|
// Why v0.12.7 is in the fixture: parseVersionSegment coerces unparseable
|
|
// segments to 0, so before the shape guard these tokens became [0] and
|
|
// prefix-matched the 0.x install — seeding a decade-old node.
|
|
const home = makeNvmHome({
|
|
versions: ['v0.12.7', 'v24.18.0', 'v26.7.0'],
|
|
defaultAlias: token
|
|
})
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v26.7.0', 'bin'))
|
|
})
|
|
|
|
it('still treats a numeric default as a version prefix, matching nvm', () => {
|
|
// The guard must reject non-versions without rejecting legitimate prefixes:
|
|
// real nvm resolves `0` to an installed v0.x.
|
|
const home = makeNvmHome({ versions: ['v0.12.7', 'v24.18.0'], defaultAlias: '0' })
|
|
expect(seededNvmDir(home)).toBe(join(home, '.nvm', 'versions', 'node', 'v0.12.7', 'bin'))
|
|
})
|
|
|
|
it('still finds a CLI that lives outside the default version', () => {
|
|
// Ordering must be a preference, not a restriction: the other versions stay
|
|
// as fallbacks so a CLI installed elsewhere is still reachable.
|
|
const home = makeNvmHome({
|
|
versions: ['v24.18.0', 'v26.7.0'],
|
|
defaultAlias: '24',
|
|
cliIn: 'v26.7.0'
|
|
})
|
|
expect(resolveCliCommand('codex', { platform: 'darwin', pathEnv: '', homePath: home })).toBe(
|
|
join(home, '.nvm', 'versions', 'node', 'v26.7.0', 'bin', 'codex')
|
|
)
|
|
})
|
|
})
|