From ead0a4ceb4d2482a0bad37f342611022425b1922 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 16 Aug 2026 15:00:23 +0000 Subject: [PATCH] fix(coding-agent): resolved update target from running binary, not PATH launcher resolveUpdateTargetFromPath gated the symlink->realpath resolution on allowPackageManagers, so binary-only releases (a major bump or an explicit omp.dist: "binary") wrote to the raw PATH symlink instead of the binary it resolves to. On an admin shared install where /usr/local/bin/omp is a root-owned symlink into a group-writable dir this either failed with EACCES (writing .new into the root-owned dir) or replaced the symlink with a full copy, stranding the shared install behind a split-brain second binary. Target selection no longer depends on the release's distribution channel: a foreign symlink (a non-manager alias, an admin symlink into a shared install) resolves to its real binary and the launcher is left intact in every channel. A package-manager launcher (bun/npm) keeps its deliberate in-place takeover on a binary-only release, detected by re-classifying the launcher as if managers were allowed rather than by the channel flag. The manager bin dirs are probed in the binary channel only when the launcher is a symlink, so plain-file installs stay probe-free. Fixes #8732 --- packages/coding-agent/CHANGELOG.md | 4 ++ packages/coding-agent/src/cli/update-cli.ts | 55 ++++++++++++++++--- packages/coding-agent/test/update-cli.test.ts | 46 ++++++++++++++++ 3 files changed, 98 insertions(+), 7 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4901d84f7..ac753ffe5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `omp update` writing to the PATH launcher instead of the running binary on binary-only releases (major bumps or `omp.dist: "binary"`): a foreign symlink — e.g. an admin symlink into a shared install — now resolves to its real binary in every distribution channel, avoiding an `EACCES` on a root-owned link directory or a split-brain copy that shadows the shared install. Package-manager launchers keep their deliberate in-place takeover. ([#8732](https://github.com/can1357/oh-my-pi/issues/8732)) + ## [17.3.5] - 2026-08-16 ### Added diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 081707fc6..c13200edf 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -441,6 +441,14 @@ function tryRealpath(p: string): string | undefined { } } +function isSymlinkPath(p: string): boolean { + try { + return fs.lstatSync(p).isSymbolicLink(); + } catch { + return false; + } +} + function isPathInDirectoryLexical(filePath: string, directoryPath: string): boolean { const normalizedPath = normalizePathForComparison(path.resolve(filePath)); const normalizedDirectory = normalizePathForComparison(path.resolve(directoryPath)); @@ -509,6 +517,14 @@ interface UpdateMethodResolutionOptions { * preserves a global package symlink instead of resolving into its checkout. */ ompLinkTarget?: string; + /** + * Whether package-manager routing (bun/npm) is permitted. Binary-only + * releases pass `false`: a manager launcher then resolves to `"binary"` and + * is taken over in place rather than reinstalled through its manager. Defaults + * to `true` in {@link resolveUpdateMethod} so callers that only classify need + * not set it. + */ + allowPackageManagers?: boolean; } type UpdateTarget = @@ -525,6 +541,7 @@ function resolveUpdateMethod( options: UpdateMethodResolutionOptions = {}, ): UpdateMethod { const { + allowPackageManagers = true, bunGlobalDir, homebrewPrefix, miseBinDirs = [], @@ -555,6 +572,7 @@ function resolveUpdateMethod( globalBinDir: bunBinDir, }); if ( + allowPackageManagers && bunBinDir && isPathInDirectory(ompPath, bunBinDir) && !isStandaloneRegularFile && @@ -564,6 +582,7 @@ function resolveUpdateMethod( } const npmNodeModulesDir = resolveNpmGlobalNodeModulesDir(npmBinDir); if ( + allowPackageManagers && npmBinDir && isPathInDirectory(ompPath, npmBinDir) && !isStandaloneRegularFile && @@ -611,10 +630,25 @@ export function resolveUpdateTargetFromPath( ompLinkTarget, }); if (method === "binary") { - // A package-manager-enabled update follows a foreign alias to replace - // its standalone binary. Binary-only releases intentionally replace the - // selected manager launcher in place. - const binaryPath = options.allowPackageManagers && ompIsSymlink ? (ompRealpath ?? ompPath) : ompPath; + // A symlinked launcher created by bun/npm is taken over in place on a + // binary-only release: routing through the manager is impossible, so the + // standalone binary replaces the launcher and keeps the PATH entry live. + // Every other symlink — a foreign alias, or an admin symlink into a + // shared install — is self-healing: update the real binary it resolves + // to and leave the launcher untouched, in every distribution channel. + // The old channel gate clobbered these foreign launchers on binary-only + // releases (EACCES on a root-owned link dir, or a stale split-brain copy + // of the binary shadowing the shared install). + const managerLauncher = + ompIsSymlink && + !options.allowPackageManagers && + resolveUpdateMethod(ompPath, bunBinDir, { + ...options, + allowPackageManagers: true, + ompIsRegularFile, + ompLinkTarget, + }) !== "binary"; + const binaryPath = ompIsSymlink && !managerLauncher ? (ompRealpath ?? ompPath) : ompPath; return { method, path: binaryPath, replacesSymlink: ompIsSymlink && binaryPath === ompPath }; } if (method === "bun" || method === "npm") return { method, path: ompPath }; @@ -630,18 +664,25 @@ export function resolveUpdateTargetFromPath( * binaries and stay valid regardless of how the release is distributed. */ async function resolveUpdateTarget(options: { allowPackageManagers: boolean }): Promise { - const bunBinDir = options.allowPackageManagers ? await getBunGlobalBinDir() : undefined; - const npmBinDir = options.allowPackageManagers ? await getNpmGlobalBinDir() : undefined; const homebrewPrefix = await getHomebrewFormulaPrefix(); const miseAvailable = $which("mise") !== undefined; const miseBinDirs = miseAvailable ? await getMiseBinDirs() : []; const miseDataDir = miseAvailable ? getMiseDataDir() : undefined; const ompPath = resolveOmpPath(); + // Binary-only releases skip package-manager routing, but a symlinked + // launcher still needs the manager bin dirs to tell a bun/npm launcher + // (taken over in place) from a foreign symlink (resolved to its real + // binary). A plain-file install never needs the distinction, so the common + // case stays probe-free. + const probeManagers = options.allowPackageManagers || (ompPath !== undefined && isSymlinkPath(ompPath)); + const bunBinDir = probeManagers ? await getBunGlobalBinDir() : undefined; + const npmBinDir = probeManagers ? await getNpmGlobalBinDir() : undefined; + if (ompPath) { return resolveUpdateTargetFromPath(ompPath, bunBinDir, { allowPackageManagers: options.allowPackageManagers, - bunGlobalDir: options.allowPackageManagers ? process.env.BUN_INSTALL_GLOBAL_DIR : undefined, + bunGlobalDir: probeManagers ? process.env.BUN_INSTALL_GLOBAL_DIR : undefined, homebrewPrefix, miseBinDirs, miseDataDir, diff --git a/packages/coding-agent/test/update-cli.test.ts b/packages/coding-agent/test/update-cli.test.ts index 4e0e5d70e..a479fd9f2 100644 --- a/packages/coding-agent/test/update-cli.test.ts +++ b/packages/coding-agent/test/update-cli.test.ts @@ -247,6 +247,52 @@ describe("update-cli install target detection", () => { expect(target).toEqual({ method: "binary", path: standalonePath, replacesSymlink: false }); }); + it("resolves a foreign symlink to its real binary on a binary-only release instead of clobbering the launcher", async () => { + // Admin shared-install layout: a non-manager symlink in PATH points into + // a shared install dir. On a binary-only release the target must still be + // the resolved binary, not the launcher — otherwise the update writes + // beside a root-owned symlink (EACCES) or replaces it with a split-brain + // copy that shadows the shared install (#8732). + const dir = await makeTempDir(); + const sharedBinDir = path.join(dir, "opt", "omp", "bin"); + const standalonePath = path.join(sharedBinDir, "omp"); + const launcherDir = path.join(dir, "usr", "local", "bin"); + const launcherPath = path.join(launcherDir, "omp"); + await fs.mkdir(sharedBinDir, { recursive: true }); + await fs.mkdir(launcherDir, { recursive: true }); + await Bun.write(standalonePath, "binary"); + await fs.symlink(standalonePath, launcherPath); + + const target = resolveUpdateTargetFromPath(launcherPath, undefined, { + allowPackageManagers: false, + }); + + expect(target).toEqual({ method: "binary", path: standalonePath, replacesSymlink: false }); + expect(await fs.readlink(launcherPath)).toBe(standalonePath); + }); + + it("takes over a package-manager launcher in place on a binary-only release", async () => { + // A bun/npm-managed launcher symlinks into the manager's node_modules. + // A forced binary release cannot route through the manager, so the + // launcher is deliberately replaced in place, keeping the PATH entry live. + const dir = await makeTempDir(); + const npmPrefix = path.join(dir, ".npm-global"); + const npmBinDir = path.join(npmPrefix, "bin"); + const managedBinary = path.join(npmPrefix, "lib", "node_modules", "@oh-my-pi", "pi-coding-agent", "omp"); + const aliasPath = path.join(npmBinDir, "omp"); + await fs.mkdir(npmBinDir, { recursive: true }); + await fs.mkdir(path.dirname(managedBinary), { recursive: true }); + await Bun.write(managedBinary, "binary"); + await fs.symlink(managedBinary, aliasPath); + + const target = resolveUpdateTargetFromPath(aliasPath, undefined, { + allowPackageManagers: false, + npmBinDir, + }); + + expect(target).toEqual({ method: "binary", path: aliasPath, replacesSymlink: true }); + }); + it("keeps a split-root Bun-linked checkout under Bun management instead of overwriting its script", async () => { const dir = await makeTempDir(); const bunBinDir = path.join(dir, "bun-bin");