fix(cli): replace preferred-pm with local implementation#984
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Claude finished @binoy14's task in 5m 10s —— View job Review
Previously reported infinite loop (
return detectFromLockFile(dir) ?? 'yarn'Any directory with a
The
Both paths work correctly for all test cases but the intent is non-obvious. A comment on that line would help future readers.
while (dir !== root) { ... }
return startDirThe loop exits before checking Test coverage is thorough — 22 cases including the nested workspace walk-up, malformed JSON, YAML/JSON Changeset —
|
📦 Bundle Stats —
|
| Metric | Value | vs main (39834ee) |
|---|---|---|
| Internal (raw) | 2.1 KB | - |
| Internal (gzip) | 799 B | - |
| Bundled (raw) | 10.94 MB | - |
| Bundled (gzip) | 2.05 MB | - |
| Import time | 870ms | +16ms, +1.9% |
bin:sanity
| Metric | Value | vs main (39834ee) |
|---|---|---|
| Internal (raw) | 975 B | - |
| Internal (gzip) | 460 B | - |
| Bundled (raw) | 9.84 MB | - |
| Bundled (gzip) | 1.77 MB | - |
| Import time | 2.05s | +56ms, +2.8% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — @sanity/cli-core
Compared against main (39834eef)
| Metric | Value | vs main (39834ee) |
|---|---|---|
| Internal (raw) | 93.8 KB | - |
| Internal (gzip) | 21.9 KB | - |
| Bundled (raw) | 21.62 MB | - |
| Bundled (gzip) | 3.42 MB | - |
| Import time | 823ms | +10ms, +1.3% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — create-sanity
Compared against main (39834eef)
| Metric | Value | vs main (39834ee) |
|---|---|---|
| Internal (raw) | 976 B | - |
| Internal (gzip) | 507 B | - |
| Bundled (raw) | 50.7 KB | - |
| Bundled (gzip) | 12.6 KB | - |
| Import time | ❌ ChildProcess denied: node | - |
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
Coverage Delta
Comparing 2 changed files against main @ Overall Coverage
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndows Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… match When a nested package.json has a workspaces field whose globs don't match the target path, continue traversing parent directories instead of returning null. Also simplify error handling in detectFromNodeModules to swallow all read errors for best-effort detection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b809ca7. Configure here.
The continue statement skipped dir = path.dirname(dir) at the bottom of the while loop, causing an infinite loop when a workspace's globs didn't match the target path. Removing it lets the code fall through to the directory advancement naturally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chmod doesn't enforce read permissions on Windows, so the file remains readable and the test returns 'pnpm' instead of falling through to 'npm'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mttdnt
left a comment
There was a problem hiding this comment.
LGTM, seems close enough to the npm packages, just a couple nits
| import picomatch from 'picomatch' | ||
|
|
||
| import {toForwardSlashes} from '../toForwardSlashes.js' | ||
| import {type PackageManager} from './packageManagerChoice.js' |
There was a problem hiding this comment.
should this be in a separate types file just to avoid the circular import
| }) | ||
| }) | ||
|
|
||
| describe('workspace root detection', () => { |
There was a problem hiding this comment.
In this block maybe add a test for a malformed package.json in detectFromWorkspaceRoot
Address PR review feedback: inline DetectablePackageManager type to break circular dependency between preferredPm and packageManagerChoice, and add test covering malformed package.json handling in workspace walk-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): replace preferred-pm with local implementation (#984) * fix(cli): replace preferred-pm with local implementation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): address review feedback for preferredPm Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): detect pnpm-workspace.yaml in parent directory walk-up Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): normalize path separators for workspace glob matching on windows Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): continue workspace walk-up when inner workspace globs don't match When a nested package.json has a workspaces field whose globs don't match the target path, continue traversing parent directories instead of returning null. Also simplify error handling in detectFromNodeModules to swallow all read errors for best-effort detection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): remove continue that caused infinite loop in workspace walk-up The continue statement skipped dir = path.dirname(dir) at the bottom of the while loop, causing an infinite loop when a workspace's globs didn't match the target path. Removing it lets the code fall through to the directory advancement naturally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): skip chmod-based permission test on Windows chmod doesn't enforce read permissions on Windows, so the file remains readable and the test returns 'pnpm' instead of falling through to 'npm'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): remove circular import and add malformed package.json test Address PR review feedback: inline DetectablePackageManager type to break circular dependency between preferredPm and packageManagerChoice, and add test covering malformed package.json handling in workspace walk-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: split e2e workflow into PR and scheduled runs (#995) Move registry-mode e2e tests and Slack failure notifications into a dedicated hourly scheduled workflow. The PR/push workflow now only runs in pack mode. Post-release dispatch in release.yml updated to target the new scheduled workflow with `latest` instead of a specific version. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(cli): replace default namespace imports with named imports (#993) Use named imports for semver, nock, and typescript to resolve 167 import-x/no-named-as-default-member lint warnings. Also replace one `as any` with a proper type cast. CJS modules (json5, dotenv, tar-fs, tar-stream) are left unchanged since they don't support named ESM imports at runtime. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): default to TypeScript in unattended mode for sanity init (#1004) * fix(cli): default to TypeScript in unattended mode for sanity init When running sanity init in unattended mode (non-interactive terminal or --yes flag), the CLI now defaults to TypeScript when --typescript or --no-typescript is not explicitly provided, matching the interactive prompt default. Uses the existing flagOrDefault/shouldPrompt helpers already used by initNextJs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: update auto-generated changeset for PR #1004 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com> * refactor(cli): extract init command into framework-agnostic initAction (#999) * refactor(cli): extract init command body into framework-agnostic initAction Moves ~1100 lines of logic out of the oclif InitCommand into actions/init/initAction.ts, leaving init.ts as a thin wrapper that maps oclif flags → InitOptions and catches InitError. Flow files (initApp, initStudio, initNextJs, scaffoldTemplate) now accept `options: InitOptions` instead of individual flag params, and throw InitError directly instead of receiving an `error` callback. Zero test assertion changes — all 14 init test files pass as-is. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(cli): clean up init telemetry types - Make `visibility` optional in `CreateOrSelectDatasetStep` (can be undefined in unattended mode) - Remove dead `ConfigureAppProjectStep` interface (no trace.log call exists for it) - Remove unnecessary `as 'private' | 'public'` cast now that the type accepts undefined Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review feedback — restore telemetry, fix env path, add tests - Restore configureAppProject trace calls dropped during extraction (thread `trace` param into promptForAppTemplateSetup) - Replace `throw new InitError('', 0)` with plain `return` on --env path (it's a normal success path, not an error) - Remove exitCode===0 special-case from command wrapper - Add ConfigureAppProjectStep to telemetry union type - Port 4 new test files from #845: flagsToInitOptions, initAction, initError, init.command - Update 2 existing tests: --env path no longer throws Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(cli): close remaining oclif decoupling gaps in init flow files - bootstrapTemplate: make overwriteFiles/useTypeScript optional - initHelpers: use InitContext['output'] instead of Output - initNextJs: throw InitError instead of output.error(), pass options directly - initStudio: dynamic import for ImportDatasetCommand, use workDir over process.cwd() - initApp: use workDir over process.cwd() - scaffoldTemplate: remove as boolean casts and unnecessary try/catch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review feedback — clean up test names, static import, tighten useTypeScript type - Rename describe blocks to simpler names (mcpMode resolution, error handling) - Use static import for InitError instead of dynamic import - Make useTypeScript a required boolean since flagOrDefault always returns boolean Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: update selectTemplate tests for refactored options signature Tests from #1004 used the old flat-argument signature; wrap in options object to match the current selectTemplate interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(cli): split initAction.ts into plan/ and project/ modules Extract helper functions from the 1175-line initAction.ts into one-function-per-file modules under plan/ and project/ subdirectories. initAction.ts now contains only the main orchestrator, flag validation, and auth — down to ~300 lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: forward datasetDefault to app template dataset creation The refactor from class methods to standalone functions dropped the implicit this.flags['dataset-default'] read that getOrCreateDataset relied on. Thread datasetDefault through getProjectDetails → promptForAppTemplateSetup → getOrCreateDataset so --dataset-default applies to app templates again. Adds a regression test exercising the app template + --dataset-default code path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Binoy Patel <me@binoy.io> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com> Co-authored-by: Matthew Dent <mttdnt@gmail.com>
* fix(cli): replace preferred-pm with local implementation (#984) * fix(cli): replace preferred-pm with local implementation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): address review feedback for preferredPm Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): detect pnpm-workspace.yaml in parent directory walk-up Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): normalize path separators for workspace glob matching on windows Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): continue workspace walk-up when inner workspace globs don't match When a nested package.json has a workspaces field whose globs don't match the target path, continue traversing parent directories instead of returning null. Also simplify error handling in detectFromNodeModules to swallow all read errors for best-effort detection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): remove continue that caused infinite loop in workspace walk-up The continue statement skipped dir = path.dirname(dir) at the bottom of the while loop, causing an infinite loop when a workspace's globs didn't match the target path. Removing it lets the code fall through to the directory advancement naturally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): skip chmod-based permission test on Windows chmod doesn't enforce read permissions on Windows, so the file remains readable and the test returns 'pnpm' instead of falling through to 'npm'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): remove circular import and add malformed package.json test Address PR review feedback: inline DetectablePackageManager type to break circular dependency between preferredPm and packageManagerChoice, and add test covering malformed package.json handling in workspace walk-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: split e2e workflow into PR and scheduled runs (#995) Move registry-mode e2e tests and Slack failure notifications into a dedicated hourly scheduled workflow. The PR/push workflow now only runs in pack mode. Post-release dispatch in release.yml updated to target the new scheduled workflow with `latest` instead of a specific version. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(cli): replace default namespace imports with named imports (#993) Use named imports for semver, nock, and typescript to resolve 167 import-x/no-named-as-default-member lint warnings. Also replace one `as any` with a proper type cast. CJS modules (json5, dotenv, tar-fs, tar-stream) are left unchanged since they don't support named ESM imports at runtime. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli): default to TypeScript in unattended mode for sanity init (#1004) * fix(cli): default to TypeScript in unattended mode for sanity init When running sanity init in unattended mode (non-interactive terminal or --yes flag), the CLI now defaults to TypeScript when --typescript or --no-typescript is not explicitly provided, matching the interactive prompt default. Uses the existing flagOrDefault/shouldPrompt helpers already used by initNextJs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: update auto-generated changeset for PR #1004 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com> * refactor(cli): extract init command into framework-agnostic initAction (#999) * refactor(cli): extract init command body into framework-agnostic initAction Moves ~1100 lines of logic out of the oclif InitCommand into actions/init/initAction.ts, leaving init.ts as a thin wrapper that maps oclif flags → InitOptions and catches InitError. Flow files (initApp, initStudio, initNextJs, scaffoldTemplate) now accept `options: InitOptions` instead of individual flag params, and throw InitError directly instead of receiving an `error` callback. Zero test assertion changes — all 14 init test files pass as-is. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(cli): clean up init telemetry types - Make `visibility` optional in `CreateOrSelectDatasetStep` (can be undefined in unattended mode) - Remove dead `ConfigureAppProjectStep` interface (no trace.log call exists for it) - Remove unnecessary `as 'private' | 'public'` cast now that the type accepts undefined Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review feedback — restore telemetry, fix env path, add tests - Restore configureAppProject trace calls dropped during extraction (thread `trace` param into promptForAppTemplateSetup) - Replace `throw new InitError('', 0)` with plain `return` on --env path (it's a normal success path, not an error) - Remove exitCode===0 special-case from command wrapper - Add ConfigureAppProjectStep to telemetry union type - Port 4 new test files from #845: flagsToInitOptions, initAction, initError, init.command - Update 2 existing tests: --env path no longer throws Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(cli): close remaining oclif decoupling gaps in init flow files - bootstrapTemplate: make overwriteFiles/useTypeScript optional - initHelpers: use InitContext['output'] instead of Output - initNextJs: throw InitError instead of output.error(), pass options directly - initStudio: dynamic import for ImportDatasetCommand, use workDir over process.cwd() - initApp: use workDir over process.cwd() - scaffoldTemplate: remove as boolean casts and unnecessary try/catch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review feedback — clean up test names, static import, tighten useTypeScript type - Rename describe blocks to simpler names (mcpMode resolution, error handling) - Use static import for InitError instead of dynamic import - Make useTypeScript a required boolean since flagOrDefault always returns boolean Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: update selectTemplate tests for refactored options signature Tests from #1004 used the old flat-argument signature; wrap in options object to match the current selectTemplate interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(cli): split initAction.ts into plan/ and project/ modules Extract helper functions from the 1175-line initAction.ts into one-function-per-file modules under plan/ and project/ subdirectories. initAction.ts now contains only the main orchestrator, flag validation, and auth — down to ~300 lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: forward datasetDefault to app template dataset creation The refactor from class methods to standalone functions dropped the implicit this.flags['dataset-default'] read that getOrCreateDataset relied on. Thread datasetDefault through getProjectDetails → promptForAppTemplateSetup → getOrCreateDataset so --dataset-default applies to app templates again. Adds a regression test exercising the app template + --dataset-default code path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Binoy Patel <me@binoy.io> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com> Co-authored-by: Matthew Dent <mttdnt@gmail.com>

Description
Replaces the
preferred-pmnpm dependency with a local implementation to fix compatibility with yarn v1 on Node 20. Thepreferred-pm@5.0.0package requiresnode >=22.13, which breaks yarn v1 Classic since it enforces engine checks by default.See #983 for context on the yarn v1 + Node 20 incompatibility.
Changes:
preferred-pmdependency from@sanity/clipreferredPmmodule that detects package managers via lock files, workspace configurations, andnode_modulesmarkerspackages/child/src).modules.yamlparsing to handle both YAML and JSON formatsWhat to review
packages/@sanity/cli/src/util/packageManager/preferredPm.ts— the local implementationpackages/@sanity/cli/src/util/packageManager/__tests__/preferredPm.test.ts— unit tests (22 cases)packages/@sanity/cli/src/util/packageManager/packageManagerChoice.ts— updated import and usageTesting
Unit tests cover: lock file detection (all 6 lock file types), parent directory pnpm-lock walk-up, workspace root detection (yarn/npm/pnpm/bun workspaces, subdirectory paths, non-matching paths,
workspaces.packagesformat),node_modulesmarker detection (YAML and JSON.modules.yamlformats), and empty directory fallback.Note
Medium Risk
Changes CLI package-manager auto-detection logic and removes a third-party dependency, which can affect dependency installation/upgrade flows across different workspace layouts. Risk is mitigated by extensive new unit tests, but behavior differences in edge cases are still possible.
Overview
Fixes Yarn v1 on Node 20 by removing the
preferred-pmdependency (and its Node 22+ engine requirement) and switchinggetPackageManagerChoiceto use a new in-repopreferredPmimplementation.The new detector infers the preferred package manager from lockfiles, pnpm lock/workspace files in parent directories, workspace root
package.jsonglobs, andnode_modulesmarkers (including more tolerant.modules.yamlparsing), with a comprehensive newvitestsuite to cover these cases.Reviewed by Cursor Bugbot for commit a000164. Bugbot is set up for automated code reviews on this repo. Configure here.