mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 00:03:15 +00:00
fix(agents): correct the ordering-parity claims and widen the seed-leak guard
Round-3 review found two docstring claims that are false as written and one guard that only asserted a third of its list. - system-cli-install-dirs.ts claimed a CLI in two of these dirs resolves the same here as in the packaged PATH seed. True inside the system block, false across it: `claude` in both ~/.local/bin and /opt/homebrew/bin resolves to ~/.local/bin via the fallback (getBaseVersionManagerDirectories leads) and to /opt/homebrew/bin via the seed, which appends ~/.local/bin last. Scope the claim to the block and name the gap instead of asserting it away. No behavior change: closing it would hoist a system dir over a version-manager one (#18234). - posix-version-manager-bin-dirs.ts justified moving "/usr/local/bin" after the nvm glob with "a version manager still wins in the guest, as it does natively". The glob expands lexicographically; native orders nvm dirs default-alias-first (#10932), so that is not parity. Record the move as the one behavior change in the file and bound it: entries are appended behind a resolved login PATH, and both consumers only test presence. - The #18234 seed-leak guard asserted only /opt/homebrew/bin and /usr/local/bin were absent from getVersionManagerBinPaths, leaving the four other new dirs unpinned. It now spells out all seven across darwin and linux -- spelled out rather than derived from getSystemCliInstallDirectories, which would pass vacuously against exactly the refactor it guards. Tests 8 passed (8); 5 failed / 3 passed with the production hunks reverted to origin/main. tsc node + cli clean, oxlint clean.
This commit is contained in:
@@ -163,9 +163,24 @@ describe('agent CLI install-dir fallback', () => {
|
||||
// patchPackagedProcessPath and the CLI's addAgentNodePaths, so a system dir
|
||||
// leaking into it would re-rank binaries the user already has (#18234).
|
||||
it('keeps system install dirs out of the PATH seed list', () => {
|
||||
const seeded = getVersionManagerBinPaths({ platform: 'darwin', homePath: '/Users/tester' })
|
||||
expect(seeded).not.toContain('/opt/homebrew/bin')
|
||||
expect(seeded).not.toContain('/usr/local/bin')
|
||||
for (const platform of ['darwin', 'linux'] as const) {
|
||||
const home = platform === 'darwin' ? '/Users/tester' : '/home/tester'
|
||||
const seeded = getVersionManagerBinPaths({ platform, homePath: home })
|
||||
// Spelled out, not derived from the list under test: a guard that iterates
|
||||
// getSystemCliInstallDirectories passes vacuously if that list is emptied
|
||||
// into getBaseVersionManagerDirectories, which is the leak it guards.
|
||||
for (const directory of [
|
||||
'/opt/homebrew/bin',
|
||||
'/usr/local/bin',
|
||||
'/snap/bin',
|
||||
'/home/linuxbrew/.linuxbrew/bin',
|
||||
'/nix/var/nix/profiles/default/bin',
|
||||
join(home, '.nix-profile', 'bin'),
|
||||
join(home, '.opencode', 'bin')
|
||||
]) {
|
||||
expect(seeded).not.toContain(directory)
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
// Why through this entry point: it is what the `orca` CLI's agent detection
|
||||
|
||||
@@ -14,9 +14,14 @@
|
||||
* on native does not. `/opt/homebrew` stays out because a WSL guest is Linux,
|
||||
* where Homebrew installs to the Linuxbrew prefix below.
|
||||
*
|
||||
* Version-manager dirs lead and the system block trails, in the same relative
|
||||
* order as the other two lists, so a CLI present in two of them resolves to the
|
||||
* same binary in the guest as it does natively.
|
||||
* Version-manager dirs lead and the system block trails, which took the one
|
||||
* behavior change here: `/usr/local/bin` moved from before the nvm glob to
|
||||
* after it, so the guest ranks a version manager over a system install the way
|
||||
* native does. Bounded, not free: every entry is APPENDED behind a resolved
|
||||
* login PATH, so this can only re-rank a command that BOTH consumers would
|
||||
* otherwise miss, and both only test presence. Not full parity either -- the
|
||||
* glob expands lexicographically, while native orders nvm dirs
|
||||
* default-alias-first (#10932).
|
||||
*
|
||||
* Each entry is quoted so a `$HOME` containing a space cannot word-split into
|
||||
* a relative path -- except the nvm glob, where only the prefix is quoted so
|
||||
|
||||
@@ -6,13 +6,19 @@ import { join } from 'node:path'
|
||||
* (#829 named `~/.opencode/bin` as the motivating case, but only for the
|
||||
* login-shell probe; the fallback used when that probe fails never gained it).
|
||||
*
|
||||
* Ordered to match what `patchPackagedProcessPath` appends to PATH, so a CLI
|
||||
* present in two of these dirs resolves to the same binary here, in the packaged
|
||||
* PATH scan, and in `POSIX_VERSION_MANAGER_BIN_DIRS`. Three deliberate gaps vs
|
||||
* that seed: the `sbin` dirs and the generic `~/bin` / `~/.local/bin` (no agent
|
||||
* CLI installer targets those, and `~/.local/bin` is already a version-manager
|
||||
* dir here); `~/.vite-plus/bin`, which no probed agent command maps to; and
|
||||
* `/opt/homebrew` off darwin, since a Linux box's brew prefix is Linuxbrew's.
|
||||
* Ordered to match the system block `patchPackagedProcessPath` appends to PATH,
|
||||
* so a CLI present in two of *these* dirs resolves to the same binary here, in
|
||||
* the packaged PATH scan, and in `POSIX_VERSION_MANAGER_BIN_DIRS`. That parity
|
||||
* stops at the block boundary and is not claimed across it: the seed appends
|
||||
* `~/.local/bin` after this block, while here it arrives ahead of it from
|
||||
* `getBaseVersionManagerDirectories`, so a `claude` installed in both
|
||||
* `~/.local/bin` and `/opt/homebrew/bin` resolves to the former via this
|
||||
* fallback and the latter via the seeded PATH. Pre-existing, and left alone
|
||||
* because closing it means hoisting a system dir over a version-manager one.
|
||||
*
|
||||
* Deliberate gaps vs that seed: the `sbin` dirs and the generic `~/bin`;
|
||||
* `~/.vite-plus/bin`, which no probed agent command maps to; and `/opt/homebrew`
|
||||
* off darwin, since a Linux box's brew prefix is Linuxbrew's.
|
||||
*
|
||||
* Lookup-only, deliberately outside `getBaseVersionManagerDirectories`: that
|
||||
* list is PREPENDED to PATH by `getVersionManagerBinPaths` callers, and hoisting
|
||||
|
||||
Reference in New Issue
Block a user