diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2d0cd08f4..b1b759819 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed the parent TUI stalling after a subagent submits its result until terminal focus or resize wakes the event loop ([#8462](https://github.com/can1357/oh-my-pi/issues/8462)). +- Fixed `omp update` misclassifying foreign npm/bun bin aliases while preserving package-manager ownership for globally linked checkouts ([#8468](https://github.com/can1357/oh-my-pi/issues/8468)). - Fixed `read` hashline headers collapsing nested in-workspace paths to the bare basename, which let a same-basename file at the session cwd capture a verbatim follow-up `edit` and deterministically reject it with `hash is not from this session`. Headers now retain the workspace-relative path (e.g. `[src/settings.json#0063]`) ([#8482](https://github.com/can1357/oh-my-pi/issues/8482)). ## [17.3.1] - 2026-08-13 diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 1ed9f3c18..081707fc6 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -467,6 +467,27 @@ function isPathInDirectory(filePath: string, directoryPath: string): boolean { return isPathInDirectoryLexical(resolvedFile, dirReal); } +function isPathInManagerRoot(linkTarget: string, nodeModulesDir: string): boolean { + if (isPathInDirectoryLexical(linkTarget, nodeModulesDir)) return true; + // Resolve only the manager root. Resolving the link target itself would + // follow globally linked packages into their checkout and lose ownership. + const nodeModulesReal = tryRealpath(path.resolve(nodeModulesDir)); + return nodeModulesReal !== undefined && isPathInDirectoryLexical(linkTarget, nodeModulesReal); +} + +function resolveNpmGlobalNodeModulesDir(globalBinDir: string | undefined): string | undefined { + if (!globalBinDir) return undefined; + if (process.platform === "win32") return path.join(globalBinDir, "node_modules"); + return path.join(path.dirname(globalBinDir), "lib", "node_modules"); +} + +function isManagerOwnedBinEntry(linkTarget: string | undefined, nodeModulesDir: string | undefined): boolean { + // Non-symlink launchers and unreadable links retain the existing bin-dir + // classification. A readable link must point through the manager's exact + // global node_modules tree. + return linkTarget === undefined || (nodeModulesDir !== undefined && isPathInManagerRoot(linkTarget, nodeModulesDir)); +} + type UpdateMethod = "brew" | "mise" | "nix" | "bun" | "npm" | "binary"; interface UpdateMethodResolutionOptions { @@ -474,6 +495,8 @@ interface UpdateMethodResolutionOptions { miseBinDirs?: readonly string[]; miseDataDir?: string; npmBinDir?: string; + /** Bun's configured global package directory, independent of its bin directory. */ + bunGlobalDir?: string; /** * Whether the resolved omp path is a plain file (the standalone binary) * rather than a package-manager symlink. Stops a binary install from being @@ -481,6 +504,11 @@ interface UpdateMethodResolutionOptions { * target directory. */ ompIsRegularFile?: boolean; + /** + * Absolute path named by the bin entry's first symlink hop. This deliberately + * preserves a global package symlink instead of resolving into its checkout. + */ + ompLinkTarget?: string; } type UpdateTarget = @@ -496,7 +524,15 @@ function resolveUpdateMethod( bunBinDir: string | undefined, options: UpdateMethodResolutionOptions = {}, ): UpdateMethod { - const { homebrewPrefix, miseBinDirs = [], miseDataDir, npmBinDir, ompIsRegularFile = false } = options; + const { + bunGlobalDir, + homebrewPrefix, + miseBinDirs = [], + miseDataDir, + npmBinDir, + ompIsRegularFile = false, + ompLinkTarget, + } = options; const launcherExtension = path.extname(ompPath).toLowerCase(); const isWindowsScriptLauncher = launcherExtension === ".cmd" || launcherExtension === ".ps1" || launcherExtension === ".bat"; @@ -514,9 +550,28 @@ function resolveUpdateMethod( // (bun's .exe launcher, npm's .cmd/.ps1), so a regular file is NOT evidence // of a standalone install and the override would hijack managed installs. const isStandaloneRegularFile = ompIsRegularFile && process.platform !== "win32"; - if (bunBinDir && isPathInDirectory(ompPath, bunBinDir) && !isStandaloneRegularFile) return "bun"; - if ((npmBinDir && isPathInDirectory(ompPath, npmBinDir) && !isStandaloneRegularFile) || isWindowsScriptLauncher) + const bunNodeModulesDir = resolveBunGlobalNodeModulesDirFromLocations({ + globalDir: bunGlobalDir, + globalBinDir: bunBinDir, + }); + if ( + bunBinDir && + isPathInDirectory(ompPath, bunBinDir) && + !isStandaloneRegularFile && + isManagerOwnedBinEntry(ompLinkTarget, bunNodeModulesDir) + ) { + return "bun"; + } + const npmNodeModulesDir = resolveNpmGlobalNodeModulesDir(npmBinDir); + if ( + npmBinDir && + isPathInDirectory(ompPath, npmBinDir) && + !isStandaloneRegularFile && + isManagerOwnedBinEntry(ompLinkTarget, npmNodeModulesDir) + ) { return "npm"; + } + if (isWindowsScriptLauncher) return "npm"; return "binary"; } @@ -527,6 +582,44 @@ export function resolveUpdateMethodForTest( ): UpdateMethod { return resolveUpdateMethod(ompPath, bunBinDir, options); } + +/** Resolve an update target from the concrete PATH entry selected by the shell. */ +export function resolveUpdateTargetFromPath( + ompPath: string, + bunBinDir: string | undefined, + options: UpdateMethodResolutionOptions & { allowPackageManagers: boolean }, +): UpdateTarget { + let ompIsRegularFile = false; + let ompIsSymlink = false; + let ompLinkTarget: string | undefined; + let ompRealpath: string | undefined; + try { + const stat = fs.lstatSync(ompPath); + ompIsRegularFile = stat.isFile() && !stat.isSymbolicLink(); + ompIsSymlink = stat.isSymbolicLink(); + if (ompIsSymlink) { + const rawTarget = fs.readlinkSync(ompPath); + const linkDir = path.dirname(ompPath); + ompLinkTarget = path.resolve(tryRealpath(linkDir) ?? linkDir, rawTarget); + ompRealpath = tryRealpath(ompPath); + } + } catch {} + + const method = resolveUpdateMethod(ompPath, bunBinDir, { + ...options, + ompIsRegularFile, + 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; + return { method, path: binaryPath, replacesSymlink: ompIsSymlink && binaryPath === ompPath }; + } + if (method === "bun" || method === "npm") return { method, path: ompPath }; + return { method }; +} /** * Resolve how the running install should be updated. * @@ -546,27 +639,14 @@ async function resolveUpdateTarget(options: { allowPackageManagers: boolean }): const ompPath = resolveOmpPath(); if (ompPath) { - // Package-manager installs symlink the bin entry into node_modules; the - // standalone installer writes a plain executable. When the global bin dir - // overlaps the installer's default (~/.local/bin), that file type — not - // directory containment — distinguishes a binary install from npm/bun. - let ompIsRegularFile = false; - let ompIsSymlink = false; - try { - const stat = fs.lstatSync(ompPath); - ompIsRegularFile = stat.isFile() && !stat.isSymbolicLink(); - ompIsSymlink = stat.isSymbolicLink(); - } catch {} - const method = resolveUpdateMethod(ompPath, bunBinDir, { + return resolveUpdateTargetFromPath(ompPath, bunBinDir, { + allowPackageManagers: options.allowPackageManagers, + bunGlobalDir: options.allowPackageManagers ? process.env.BUN_INSTALL_GLOBAL_DIR : undefined, homebrewPrefix, miseBinDirs, miseDataDir, npmBinDir, - ompIsRegularFile, }); - if (method === "binary") return { method, path: ompPath, replacesSymlink: ompIsSymlink }; - if (method === "bun" || method === "npm") return { method, path: ompPath }; - return { method }; } if (bunBinDir) return { method: "bun" }; @@ -795,10 +875,19 @@ async function resolveBunInstallCacheDir(): Promise { } } -export function resolveBunGlobalNodeModulesDirFromLocations( - globalBinDir: string | undefined, - cacheDir: string | undefined, -): string | undefined { +interface BunGlobalInstallLocations { + globalDir?: string; + globalBinDir?: string; + cacheDir?: string; +} + +/** Resolve Bun's global node_modules root from explicit, default, or cache locations. */ +export function resolveBunGlobalNodeModulesDirFromLocations({ + globalDir, + globalBinDir, + cacheDir, +}: BunGlobalInstallLocations): string | undefined { + if (globalDir && globalDir.length > 0) return path.join(globalDir, "node_modules"); if (globalBinDir && globalBinDir.length > 0) { return path.join(path.dirname(globalBinDir), "install", "global", "node_modules"); } @@ -812,9 +901,16 @@ async function resolveBunGlobalNodeModulesDir(cacheDir: string): Promise { expect(method).toBe("npm"); }); + it("updates the standalone binary behind a foreign npm-bin alias without replacing the alias", async () => { + const dir = await makeTempDir(); + const npmBinDir = path.join(dir, ".npm-global", "bin"); + const standalonePath = path.join(dir, ".local", "bin", "omp"); + const aliasPath = path.join(npmBinDir, "omp"); + await fs.mkdir(npmBinDir, { recursive: true }); + await Bun.write(standalonePath, "binary"); + await fs.symlink(standalonePath, aliasPath); + + const target = resolveUpdateTargetFromPath(aliasPath, undefined, { + allowPackageManagers: true, + npmBinDir, + }); + + expect(target).toEqual({ method: "binary", path: standalonePath, replacesSymlink: false }); + expect(await fs.readlink(aliasPath)).toBe(standalonePath); + }); + + it("keeps an npm-linked checkout under npm management instead of overwriting its resolved script", async () => { + const dir = await makeTempDir(); + const npmPrefix = path.join(dir, ".npm-global"); + const npmBinDir = path.join(npmPrefix, "bin"); + const packagePath = path.join(npmPrefix, "lib", "node_modules", "@oh-my-pi", "pi-coding-agent"); + const checkoutPath = path.join(dir, "checkout"); + const checkoutCli = path.join(checkoutPath, "dist", "cli.js"); + const aliasPath = path.join(npmBinDir, "omp"); + await fs.mkdir(npmBinDir, { recursive: true }); + await fs.mkdir(path.dirname(packagePath), { recursive: true }); + await Bun.write(checkoutCli, "linked checkout"); + await fs.symlink(checkoutPath, packagePath, "junction"); + await fs.symlink(path.relative(npmBinDir, path.join(packagePath, "dist", "cli.js")), aliasPath); + + const target = resolveUpdateTargetFromPath(aliasPath, undefined, { + allowPackageManagers: true, + npmBinDir, + }); + + expect(await fs.realpath(aliasPath)).toBe(checkoutCli); + expect(target).toEqual({ method: "npm", path: aliasPath }); + expect(await Bun.file(checkoutCli).text()).toBe("linked checkout"); + }); + + it("treats a Bun-bin alias into ~/.bun/custom as foreign", async () => { + const dir = await makeTempDir(); + const bunDir = path.join(dir, ".bun"); + const bunBinDir = path.join(bunDir, "bin"); + const standalonePath = path.join(bunDir, "custom", "omp"); + const aliasPath = path.join(bunBinDir, "omp"); + await fs.mkdir(bunBinDir, { recursive: true }); + await Bun.write(standalonePath, "binary"); + await fs.symlink(path.relative(bunBinDir, standalonePath), aliasPath); + + const target = resolveUpdateTargetFromPath(aliasPath, bunBinDir, { + allowPackageManagers: true, + }); + + expect(target).toEqual({ method: "binary", path: standalonePath, replacesSymlink: false }); + }); + + 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"); + const bunGlobalDir = path.join(dir, "bun-global"); + const packagePath = path.join(bunGlobalDir, "node_modules", "@oh-my-pi", "pi-coding-agent"); + const checkoutPath = path.join(dir, "checkout"); + const checkoutCli = path.join(checkoutPath, "dist", "cli.js"); + const aliasPath = path.join(bunBinDir, "omp"); + await fs.mkdir(bunBinDir, { recursive: true }); + await fs.mkdir(path.dirname(packagePath), { recursive: true }); + await Bun.write(checkoutCli, "linked checkout"); + await fs.symlink(checkoutPath, packagePath, "junction"); + await fs.symlink(path.relative(bunBinDir, path.join(packagePath, "dist", "cli.js")), aliasPath); + + const target = resolveUpdateTargetFromPath(aliasPath, bunBinDir, { + allowPackageManagers: true, + bunGlobalDir, + }); + + expect(await fs.realpath(aliasPath)).toBe(checkoutCli); + expect(target).toEqual({ method: "bun", path: aliasPath }); + expect(await Bun.file(checkoutCli).text()).toBe("linked checkout"); + }); + it("uses binary update when prioritized omp is outside bun global bin", () => { const method = resolveUpdateMethodForTest("/Users/test/.local/bin/omp", "/Users/test/.bun/bin"); @@ -437,13 +521,23 @@ describe("update-cli bun install command", () => { expect(args.some(arg => arg.startsWith("@oh-my-pi/pi-natives-"))).toBe(false); }); - it("derives global node_modules from supported bun global locations", () => { - expect(resolveBunGlobalNodeModulesDirFromLocations(path.join("home", ".bun", "bin"), undefined)).toBe( - path.join("home", ".bun", "install", "global", "node_modules"), - ); + it("derives global node_modules from supported Bun locations with the explicit global directory taking precedence", () => { expect( - resolveBunGlobalNodeModulesDirFromLocations(undefined, path.join("home", ".bun", "install", "cache")), + resolveBunGlobalNodeModulesDirFromLocations({ + globalBinDir: path.join("home", ".bun", "bin"), + }), ).toBe(path.join("home", ".bun", "install", "global", "node_modules")); + expect( + resolveBunGlobalNodeModulesDirFromLocations({ + cacheDir: path.join("home", ".bun", "install", "cache"), + }), + ).toBe(path.join("home", ".bun", "install", "global", "node_modules")); + expect( + resolveBunGlobalNodeModulesDirFromLocations({ + globalDir: path.join("root", "bun-global"), + globalBinDir: path.join("root", "bun-bin"), + }), + ).toBe(path.join("root", "bun-global", "node_modules")); }); });