fix(update): PATH-priority installer detection, asdf shims, --all#179
fix(update): PATH-priority installer detection, asdf shims, --all#179rafa-thayto wants to merge 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 8379604 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR makes Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rafa-thayto
force-pushed
the
rafa-thayto/auto-updatee
branch
from
3446aa3 to
b364003
Compare
rafa-thayto
changed the title
wyattjoh
left a comment
wyattjoh
left a comment
There was a problem hiding this comment.
Code Review — PR #179
Reviewed with Opus + Codex second-opinion validation.
Major
M1. brew upgrade clerk now auto-runs on stable; docs disagree (codex confirmed)
packages/cli-core/src/commands/update/index.ts:94-96,117-124,247 runs brew upgrade clerk via runGlobalInstall, but packages/cli-core/src/commands/update/README.md:23-31 still says Homebrew "prints brew upgrade clerk and exits (no auto-install)". Pick a semantic. If auto-upgrade is intended, update the docs and consider a separate confirmation step; running brew upgrade under a silenced spinner hides brew side-effects (dependency upgrades, autoremoves).
M2. npx clerk update / bunx clerk update silently regress (codex partial)
The old code had detectFromUserAgent returning "npm" when npm_config_user_agent=npm/.... New findClerkOnPath() cannot see the npx cache at ~/.npm/_npx/<hash>/... (not on PATH), and the fallback to process.execPath makes ownerOfBinary return null, so the command refuses with "outside any known package manager". Users who used to run npx clerk update now get a misleading install-sh error. Either preserve user-agent detection for this case, or emit an actionable "install globally first" message.
M3. Changeset is patch but PR adds --all and changes behavior
.changeset/clerk-update-path-aware.md bumps patch. A new flag plus the Homebrew behavior change lean minor.
Missed on first pass (codex caught)
- Cache poisoning on partial
--allfailure.writeUpdateCache()runs after the loop inupdate/index.ts:240-256,270even when some targets errored, so a failed run marks the latest version as cached and suppresses future update prompts until staleness. - Non-interactive auto-confirm without
--yes.README.md:77says agent mode requires--yes, butupdate/index.ts:222-223auto-confirms whenever!isHuman(), regardless of--yes.
Minor
process.env.ASDF_DATA_DIR ?? join(...)(installer.ts:167-169,191-196):??treats""as set. Use||or an explicit empty-check.queryBunPackageDiralways returns a path viasafeRealpath(installer.ts:156-159), contradicting thegetInstallerPackageDirsdocstring that says absent PMs are omitted.ownerOfBinary.startsWithon Windows (installer.ts:218-230): realpath case differences on drive letters can cause spurious "outside any known package manager" refusals.resolveTargetsfallback toprocess.execPath(update/index.ts:64-66) pairs with M2 to surface misleading errors for package-runner invocations.hashHint(update/index.ts:146-155) falls through to POSIXhash -ron Windows when$SHELLis unset.--allsummary doesn't distinguish primary-install status; a user whose shell-resolved clerk failed can miss that fact.
Positives
Excellent unit-test coverage for findClerkOnPath, ownerOfBinary, and the asdf helpers. The refuse-rather-than-guess ownership model is a real improvement over the prior silent npm fallback. The bun shim-vs-install-dir diagnosis is correct, and the realpath(mkdtemp(...)) trick to paper over /var -> /private/var is exactly the kind of detail that usually gets missed.
Before this change, `clerk update` called `detectInstaller()` on the running binary's `process.execPath`. Two bugs compounded: 1. Bun detection compared against `bun pm bin -g` (the shim dir `~/.bun/bin`), but the Bun-compiled platform binary lives in `~/.bun/install/global/node_modules/@clerk/cli-<arch>`. The match never succeeded, so detection fell through to npm. 2. The fallback `npm install -g clerk@<new>` returned exit 0 and the spinner reported success — but on machines with asdf-managed node, the update landed in `~/.asdf/shims/` while the user's shell still resolved `clerk` to the (unchanged) `~/.bun/bin/clerk`. The fix splits resolution from execution: - New `findClerkOnPath()` walks `process.env.PATH` (shell-agnostic) and returns realpath'd, deduped, executable `clerk` binaries in PATH order. - New `getInstallerPackageDirs()` returns the dir where each PM stores *packages* (not shims); the bun entry uses `$BUN_INSTALL/install/ global/node_modules` — fixing bug 1. - New `ownerOfBinary()` maps a binary path to its owning installer, returning `null` on no match. The update command treats `null` as refuse-rather-than-guess, preventing bug 2. - The update command targets the *first* `clerk` on PATH (what the user's shell will actually execute), not whatever `process.execPath` reports. Also: - `--all` flag updates every `clerk` install on PATH in one run, skipping Homebrew on non-stable channels and `null`-owned binaries with a per-install warning and summary. - Other installs are reported after every run so shadowing is visible. - Post-update `hash -r` / `rehash` hint based on `$SHELL` (skipped for fish and PowerShell, which don't cache).
asdf shims are bash scripts (not symlinks), so realpath returns the shim path itself and ownerOfBinary can't match it against any PM prefix. The PATH survey marked asdf-installed clerks as "unknown installer" and --all would skip them with a warning. resolveAsdfShim() calls `asdf which <name>` to find the underlying binary, realpaths the result, and feeds it into ownerOfBinary. Non- shim paths pass through unchanged. When asdf is absent or can't resolve, the shim path is returned and ownerOfBinary correctly reports null — preserving the refuse-on-unknown behavior. After a successful install, runs `asdf reshim <plugin>` for every asdf plugin whose binary was updated. Safety net for asdf versions that don't auto-reshim on npm install. Target splits into displayPath (what's on PATH — the shim) and resolvedPath (what ownerOfBinary consults) so users see the familiar shim path in output while install logic operates on the resolved binary.
- Remove dead detectInstaller, detectFromUserAgent, matchPmFromExecPath,
and queryPmPrefix. They were the legacy runtime-based detection path,
superseded by ownerOfBinary + findClerkOnPath. Only installer.test.ts
still imported them.
- Drop the stale "TODO for a contributor" comment in the Strategy B
section header; ownerOfBinary has been implemented.
- Rewrite getInstallerPackageDirs with Promise.all so the parallelism
is obvious at the call site.
- Replace em-dashes with regular punctuation throughout installer.ts,
update/index.ts, and update/README.md.
- Fix formatTarget double-dim: dim("unknown") was being wrapped inside
another dim(...) in the null-owner case; the inner ANSI reset broke
the outer styling. Pass the raw string instead.
rafa-thayto
force-pushed
the
rafa-thayto/auto-updatee
branch
from
7fb8b88 to
f2ebbf4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/installer.ts`:
- Around line 135-140: Replace the hardcoded POSIX path in queryNpmPackageDir()
with the output of `npm root -g` (call Bun.$`npm root -g`.quiet().nothrow(),
check exitCode, safeRealpath the stdout trimmed) so it works on Windows, and
update ownerOfBinary() to perform case-insensitive comparisons on Windows by
normalizing both paths (e.g., path.normalize()/path.resolve() and
.toLowerCase()) before matching when process.platform === 'win32'; ensure
existing null/exitCode handling remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0982a67d-4144-4122-91bb-ed5ad4b738c3
📒 Files selected for processing (7)
.changeset/clerk-update-path-aware.mdREADME.mdpackages/cli-core/src/cli-program.tspackages/cli-core/src/commands/update/README.mdpackages/cli-core/src/commands/update/index.tspackages/cli-core/src/lib/installer.test.tspackages/cli-core/src/lib/installer.ts
✅ Files skipped from review due to trivial changes (2)
- README.md
- .changeset/clerk-update-path-aware.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli-core/src/commands/update/README.md
- packages/cli-core/src/commands/update/index.ts
| async function queryNpmPackageDir(): Promise<string | null> { | ||
| const result = await Bun.$`npm prefix -g`.quiet().nothrow(); | ||
| if (result.exitCode !== 0) return null; | ||
| const prefix = result.stdout.toString().trim(); | ||
| return prefix ? await safeRealpath(join(prefix, "lib", "node_modules")) : null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only: compare npm's reported global prefix vs package root on this runner.
if command -v npm >/dev/null 2>&1; then
printf 'npm prefix -g: '
npm prefix -g
printf 'npm root -g: '
npm root -g
fiRepository: clerk/cli
Length of output: 119
🏁 Script executed:
cat -n packages/cli-core/src/lib/installer.ts | head -250Repository: clerk/cli
Length of output: 10935
🌐 Web query:
Windows npm global packages directory structure difference from Linux
💡 Result:
The primary differences in npm global packages directory structure between Windows and Linux (or Unix-like systems) stem from the default prefix and the paths under it for node_modules and executables. Default prefix: - Linux/Unix: Typically /usr/local (one level above where node is installed, e.g., /usr/local/bin/node). - Windows: %AppData%\npm (e.g., C:\Users<username>\AppData\Roaming\npm). Global node_modules location: - Linux/Unix: {prefix}/lib/node_modules (e.g., /usr/local/lib/node_modules). - Windows: {prefix}/node_modules (e.g., %AppData%\npm\node_modules). Note the absence of the 'lib' subdirectory on Windows. Global executables: - Linux/Unix: Symlinked into {prefix}/bin (e.g., /usr/local/bin). - Windows: Placed directly into {prefix} (e.g., %AppData%\npm), often as .cmd shims, and this path must be in the system PATH. Cache: - Linux/Unix: ~/.npm. - Windows: %AppData%\npm-cache or %LocalAppData%\npm-cache. These paths can be verified with npm config get prefix and npm root -g. The structure accommodates OS conventions: Unix uses hierarchical /bin, /lib paths; Windows flattens executables into the prefix directory.
Citations:
- 1: https://docs.npmjs.com/cli/v8/configuring-npm/folders
- 2: https://docs.npmjs.com/cli/v8/configuring-npm/folders/
- 3: https://docs.npmjs.com/cli/v9/configuring-npm/folders
- 4: https://docs.npmjs.com/cli/v8/using-npm/config
- 5: npm-folders contradicts default Windows global-install behavior npm/npm#17410
- 6: https://stackoverflow.com/questions/33819757/global-npm-install-location-on-windows
🏁 Script executed:
fd -t f "test|spec" packages/cli-core/src/lib/installer --type fRepository: clerk/cli
Length of output: 175
🏁 Script executed:
rg "queryNpmPackageDir|ownerOfBinary" packages/cli-core --type ts -A 5 -B 5Repository: clerk/cli
Length of output: 12087
🌐 Web query:
npm root -g command availability npm version history when introduced
💡 Result:
The npm root -g command is available in all modern versions of npm, including the current latest version (v11.x as of 2026). It displays the effective node_modules folder path in global mode when the -g or --global flag is used. The command npm root has been part of npm since at least version 1.x (early 2010s), as evidenced by its presence in documentation for v6 (2018), v7, v8, v9, v10, and v11. In v7 and earlier, the synopsis explicitly shows npm root [-g], while in v8 and later, -g is used as a flag with the global configuration option, but functions equivalently (e.g., npm root --global). No specific introduction version was found in changelogs or history, indicating it is a core, long-standing command predating npm v6.
Citations:
- 1: https://docs.npmjs.com/cli/v9/commands/npm-root
- 2: https://docs.npmjs.com/cli/v8/commands/npm-root/
- 3: https://docs.npmjs.com/cli/v7/commands/npm-root
- 4: https://docs.npmjs.com/cli/v6/commands/npm-root/
- 5: https://docs.npmjs.com/cli/v11/commands/npm-root
- 6: https://docs.npmjs.com/cli/v9/commands
- 7: https://docs.npmjs.com/cli/v8/commands/
- 8: https://docs.npmjs.com/cli/v10/commands
- 9: https://docs.npmjs.com/cli/v6/commands
Fix Windows npm path detection and case-sensitive matching before merge.
queryNpmPackageDir() hardcodes POSIX directory structure (lib/node_modules), which will fail on Windows where npm stores packages directly under the prefix. Additionally, ownerOfBinary() uses case-sensitive path matching, causing npm-installed binaries to be misclassified on Windows when path casing differs. Both prevent clerk update from working correctly on Windows.
Use npm root -g (available since npm v1.x) instead of constructing the path manually, and normalize paths to lowercase before comparison on Windows.
Proposed fix
async function queryNpmPackageDir(): Promise<string | null> {
- const result = await Bun.$`npm prefix -g`.quiet().nothrow();
+ const result = await Bun.$`npm root -g`.quiet().nothrow();
if (result.exitCode !== 0) return null;
- const prefix = result.stdout.toString().trim();
- return prefix ? await safeRealpath(join(prefix, "lib", "node_modules")) : null;
+ const dir = result.stdout.toString().trim();
+ return dir ? await safeRealpath(dir) : null;
} export function ownerOfBinary(
binaryPath: string,
installDirs: Partial<Record<Installer, string>>,
): Installer | null {
if (isHomebrewPath(binaryPath)) return "homebrew";
+ const normalizedBinaryPath = normalizeForPathCompare(binaryPath);
let best: { installer: Installer; len: number } | null = null;
for (const [pm, dir] of Object.entries(installDirs) as Array<[Installer, string]>) {
- if (!dir || !binaryPath.startsWith(dir + sep)) continue;
- if (!best || dir.length > best.len) best = { installer: pm, len: dir.length };
+ const normalizedDir = normalizeForPathCompare(dir);
+ if (!dir || !normalizedBinaryPath.startsWith(normalizedDir + sep)) continue;
+ if (!best || normalizedDir.length > best.len) {
+ best = { installer: pm, len: normalizedDir.length };
+ }
}
return best?.installer ?? null;
}
+
+function normalizeForPathCompare(path: string): string {
+ return process.platform === "win32" ? path.toLowerCase() : path;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function queryNpmPackageDir(): Promise<string | null> { | |
| const result = await Bun.$`npm prefix -g`.quiet().nothrow(); | |
| if (result.exitCode !== 0) return null; | |
| const prefix = result.stdout.toString().trim(); | |
| return prefix ? await safeRealpath(join(prefix, "lib", "node_modules")) : null; | |
| } | |
| async function queryNpmPackageDir(): Promise<string | null> { | |
| const result = await Bun.$`npm root -g`.quiet().nothrow(); | |
| if (result.exitCode !== 0) return null; | |
| const dir = result.stdout.toString().trim(); | |
| return dir ? await safeRealpath(dir) : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-core/src/lib/installer.ts` around lines 135 - 140, Replace the
hardcoded POSIX path in queryNpmPackageDir() with the output of `npm root -g`
(call Bun.$`npm root -g`.quiet().nothrow(), check exitCode, safeRealpath the
stdout trimmed) so it works on Windows, and update ownerOfBinary() to perform
case-insensitive comparisons on Windows by normalizing both paths (e.g.,
path.normalize()/path.resolve() and .toLowerCase()) before matching when
process.platform === 'win32'; ensure existing null/exitCode handling remains
intact.
- Homebrew auto-upgrades on stable channel, docs said otherwise. Update README to reflect the actual behavior. - `npx clerk update` / `bunx clerk update` used to fall back to npm; after PATH-aware detection they hit the "unknown installer" refuse branch. Detect the runner via env vars and print a global-install hint instead of the install.sh message. - Agent mode was auto-confirming whenever stdout was not a TTY, which contradicted the documented `--yes`-required behavior. Refuse in agent mode without `--yes` and print the exact command to run. - Partial `--all` failures still refreshed the update cache; only write the cache when every attempted install succeeded. - Bump changeset from patch to minor (adds `--all`, changes Homebrew behavior). - Minor: `??` -> `||` for `ASDF_DATA_DIR` (empty string now treated as unset), require the bun install dir to exist before claiming ownership of paths under it, normalize case on Windows in `ownerOfBinary`, skip the POSIX `hash -r` hint on Windows, mark the primary target in the `--all` summary.
2 participants
Fixes
clerk updatesilently writing to the wrong installer on machines with multipleclerkbinaries on PATH. Replaces runtime-based installer detection with PATH-priority-aware resolution, adds asdf shim support, and introduces an opt-in--allbatch mode.Before / after
Reporter's setup (bun + asdf-managed npm + Homebrew):
Before:
clerk update --channel canaryreported success, butclerk -vstill showed the old version. The update had landed in~/.asdf/shims/clerk(via an npm fallback), not in~/.bun/bin/clerk(the one the shell actually resolves).After: the update writes to
~/.bun/install/global/node_modules/viabun add -g, andclerk -vreflects the new version. A post-run report lists the asdf and Homebrew installs with their owners;--allupdates the asdf install too (viaasdf which→ underlying npm, plusasdf reshim nodejsafter).What the new UX looks like
Default (first-on-PATH only, other installs reported):
--all(batch + per-install summary):install.shstandalone (refuse, don't guess):Fixed
clerkon PATH (what the shell actually executes) and runs the installer that owns it. If no known installer owns the primary target, refuses with reinstall guidance instead of silently writing to a different prefix.detectInstaller()Stage 2b comparedprocess.execPathagainstbun pm bin -g(the shim dir), which never matched the Bun-compiled platform binary under~/.bun/install/global/node_modules. Replaced with path-basedownerOfBinary()that matches the install dir.realpathreturns the shim itself and nothing matches. Now resolved viaasdf which <name>; the underlying binary is owner-matched normally, andasdf reshim <plugin>runs after install as a safety net for asdf versions that don't auto-reshim.Added
--allflag. Updates everyclerkinstall on PATH in one run. Skips Homebrew on non-stable channels andnull-owned binaries with per-install warnings; summary printed at the end.clerkinstalls with their owners so shadowing is visible.hash -r(bash/zsh/sh/dash/ksh),rehash(tcsh/csh), or nothing (fish auto-rehashes, PowerShell has no cache) based on$SHELL.Version managers
realpathchases into<nvm-version>/lib/node_modules, which matches activenpm prefix -g.resolveAsdfShim+ post-installasdf reshim. Honors$ASDF_DATA_DIR. Falls back to "unknown" (refuse) when asdf isn't installed.New exports in
packages/cli-core/src/lib/installer.tsfindClerkOnPath(name?)process.env.PATH, realpaths + dedupes, filters to executable regular files (POSIX X bit / Windows PATHEXT). Shell-agnostic.getInstallerPackageDirs()ownerOfBinary(path, installDirs)null.isAsdfShimPath(path)resolveAsdfShim(path)asdf which-based resolution. Returns input unchanged when asdf is absent or can't resolve.asdfPluginFromPath(path)<asdf>/installs/<plugin>/<ver>/....asdfReshim(plugin)The existing
detectInstaller()is preserved for backward compatibility but no longer used by the update command.Test plan
bun run typecheckpassesbun run lintpasses (oxlint, 0 warnings / 0 errors)bun run testpasses (71/71 files);installer.test.tshas 53 cases including 16 new ones coveringownerOfBinary,findClerkOnPath,isAsdfShimPath,asdfPluginFromPath, andresolveAsdfShimclerk update --channel canarywrites to~/.bun/install/global/node_modulesandclerk -vreflects the new version--allupdates both bun and asdf-npm (viaasdf which) and runsasdf reshim nodejsafter--allskips Homebrew with a channel-mismatch warning when--channel canaryis usedinstall.sh-installed clerk (standalone binary) is refused with reinstall guidancehash -rhint renders on zsh/bash;rehashon tcsh; no hint on fish/pwsh