chore: switch package manager from bun to pnpm#967
Conversation
|
| "packageManager": "pnpm@10.11.0", | ||
| "pnpm": { | ||
| "patchedDependencies": { | ||
| "@stricli/core": "patches/@stricli%2Fcore@1.2.5.patch", |
There was a problem hiding this comment.
Patch version mismatch after silent dependency bump
Low Severity
The @stricli/core patch file was created for version 1.2.5 (confirmed in bun.lock), but under pnpm the patchedDependencies key omits the version ("@stricli/core" instead of bun's "@stricli/core@1.2.5"), and the lockfile now resolves to version 1.2.7. The patch applies today, but without a version qualifier in the key, any future pnpm update could bump @stricli/core further, potentially causing the patch to silently fail or produce incorrect behavior if the patched lines in dist/index.js change.
Reviewed by Cursor Bugbot for commit 124766a. Configure here.
| }, | ||
| "onlyBuiltDependencies": [ | ||
| "esbuild" | ||
| ] |
There was a problem hiding this comment.
Bug: The pnpm patch for @stricli/core uses a name-only key. If a future update changes the patched lines, the patch will silently fail, breaking the sentry api command.
Severity: MEDIUM
Suggested Fix
Change the patch key in package.json from the name-only "@stricli/core" to a versioned key like "@stricli/core@1.2.5" or a range like "@stricli/core@*". This will cause pnpm to error out if the patch cannot be applied, preventing silent failures in the future.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: package.json#L84
Potential issue: The `pnpm` patch for `@stricli/core` is configured with a name-only key
(`"@stricli/core"`) instead of a version-specific one. This patch, written for version
`1.2.5`, is being applied to `1.2.7`. While it may apply successfully now, `pnpm` v10
silently ignores patch application failures for name-only keys. If a future version of
`@stricli/core` changes the code at the patched location, the patch will fail to apply
silently. This would leave the `H` alias in `stricli`'s reserved list, causing
`src/commands/api.ts` to throw an error on load and making the `sentry api` command
completely non-functional.
Did we get this right? 👍 / 👎 to inform future reviews.
0c43a41 to
ba2f136
Compare
| "patchedDependencies": { | ||
| "@stricli/core": "patches/@stricli%2Fcore@1.2.5.patch", | ||
| "@sentry/node-core": "patches/@sentry%2Fnode-core@10.50.0.patch", | ||
| "@sentry/core": "patches/@sentry%2Fcore@10.50.0.patch" |
There was a problem hiding this comment.
Bug: Migrating to pnpm's name-only patchedDependencies keys will cause patch failures to be silently ignored on future version updates, potentially shipping unpatched packages to production.
Severity: HIGH
Suggested Fix
To ensure patch failures are not silent, change the name-only keys in patchedDependencies to use a version range, such as *. For example, change "@sentry/core" to "@sentry/core@*". According to pnpm documentation, using a version range like * will cause patch application failures to throw an error, preventing silent failures.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: package.json#L77-L80
Potential issue: The migration to `pnpm` changes `patchedDependencies` in `package.json`
to use name-only keys (e.g., `@sentry/core`) instead of version-specific ones. With
`pnpm`, name-only keys silently ignore patch application failures by default. If a
future dependency update causes a patch to become incompatible (e.g., `@sentry/core` is
updated), the patch will fail silently. This would result in the unpatched package, with
heavyweight modules that were meant to be stripped, being included in production builds,
causing significant bundle bloat or incorrect behavior without any warning.
b9a4fdd to
b1acf35
Compare
Codecov Results 📊✅ 6969 passed | Total: 6969 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 14103 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 77.06% 77.09% +0.03%
==========================================
Files 320 320 —
Lines 61608 61570 -38
Branches 0 0 —
==========================================
+ Hits 47475 47467 -8
- Misses 14133 14103 -30
- Partials 0 0 —Generated by Codecov Action |
Switch the project's package manager from bun to pnpm while keeping bun as the TS runner and test runner for now (incremental migration). Changes: - Set packageManager to pnpm@10.11.0 - Move patchedDependencies into pnpm config section - Add onlyBuiltDependencies for esbuild build script approval - Add @sentry/core and @clack/prompts as explicit devDependencies (were phantom deps hoisted by bun's flat node_modules) - Generate pnpm-lock.yaml All 6968 unit tests pass. One pre-existing test fragility (sdk.run auth test) surfaces under pnpm's strict hoisting but is unrelated to this change.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 414cd3a. Configure here.
| "lint": "bunx ultracite check", | ||
| "lint:fix": "bunx ultracite fix", | ||
| "lint": "biome check --no-errors-on-unmatched --max-diagnostics=none ./", | ||
| "lint:fix": "biome check --write --no-errors-on-unmatched --max-diagnostics=none ./", |
There was a problem hiding this comment.
Unused ultracite dependency after lint script change
Low Severity
The lint and lint:fix scripts were changed from bunx ultracite check/bunx ultracite fix to direct biome check invocations, but ultracite remains in devDependencies at line 49. No script or source file references ultracite anymore, making it a dead dependency that adds unnecessary install weight.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 414cd3a. Configure here.
## Summary Second step of the Bun → Node.js migration (follows #967). Introduces a SQLite adapter layer that decouples the codebase from direct `bun:sqlite` imports, making the database layer portable across runtimes. ### Changes - **New**: `src/lib/db/sqlite.ts` — SQLite adapter using `node:sqlite` (Node 22+) - Falls back to `bun:sqlite` when `node:sqlite` is unavailable (during Bun test runner transition only — fallback will be removed once tests migrate to Vitest) - Handles API differences: prefers `bun:sqlite`'s cached `.query()` when available, falls back to `node:sqlite`'s `.prepare()`; provides manual BEGIN/COMMIT/ROLLBACK wrapper for `node:sqlite` (which lacks native `.transaction()`) - Normalizes `get()` return value to `null` (not `undefined`) for no-row results, matching codebase convention - Exports `Database` class and `SQLQueryBindings` type - **Fixed**: `isSchemaError()` and `isReadonlyError()` in `schema.ts` — now match by error message content instead of `error.name === "SQLiteError"`, making auto-repair and readonly detection work under `node:sqlite` (which throws plain `Error`, not `SQLiteError`) - **Updated imports** in 4 source files: `db/index.ts`, `schema.ts`, `migration.ts`, `utils.ts` - **Updated imports** in 3 test files: `fix.test.ts`, `telemetry.test.ts`, `schema.test.ts` - **Updated**: `lint-rules/no-manual-transactions.grit` — excludes `db/sqlite.ts` (manual transactions are necessary in the adapter for `node:sqlite` compatibility) - **Added**: `MIGRATION-PLAN.md` documenting remaining migration steps ### Design Call sites continue using `.query(sql).get()` / `.all()` / `.run()` and `db.transaction()` exactly as before — no migration churn needed. Zero `bun:sqlite` imports remain in `src/` or `test/`.
## Summary Third step of the Bun → Node.js migration (follows #967, #970). Replaces all Bun-specific API calls in `src/` with Node.js equivalents. ### Changes **File I/O** (18 files): - `Bun.file(path).text()` → `readFile(path, "utf-8")` from `node:fs/promises` - `Bun.file(path).json()` → `JSON.parse(await readFile(path, "utf-8"))` - `Bun.file(path).exists()` → `access(path).then(() => true, () => false)` - `Bun.file(path).stat()` → `stat(path)` from `node:fs/promises` - `Bun.write(path, content)` → `writeFile(path, content)` from `node:fs/promises` - `Bun.write(dest, Bun.file(src))` → `copyFile(src, dest)` from `node:fs/promises` **Process/System** (9 files + 1 new): - `Bun.which()` → new `src/lib/which.ts` helper using `command -v` (POSIX) / `where` (Windows) - `Bun.spawn()` → `spawn()` from `node:child_process` with Promise-wrapped exit code - `Bun.spawnSync()` → `spawnSync()` from `node:child_process` - `Bun.sleep()` → `setTimeout()` from `node:timers/promises` **Utilities** (6 files): - `Bun.Glob` → `picomatch` (already a devDependency) - `Bun.randomUUIDv7()` → `uuidv7()` from `uuidv7` package - `Bun.semver.order()` → `compare()` from `semver` package ### What's NOT in this PR (Group D — separate follow-up) These Bun APIs in `bspatch.ts` and `upgrade.ts` require more careful handling: - `Bun.file().writer()` — streaming file writer (needs `fs.createWriteStream`) - `Bun.zstdCompress/DecompressSync` — zstd compression (needs `node:zlib` 22.15+) - `Bun.mmap()` — memory-mapped files (has existing fallback) - `Bun.CryptoHasher` — streaming hash (needs `crypto.createHash`) ### Test results All 7012 unit tests pass, 0 failures.
…writer) (#986) ## Summary Fourth step of the Bun → Node.js migration (follows #967, #970, #984). Replaces the remaining Bun-specific APIs in `src/` — the "Group D" items that required more careful handling. ### Changes **`src/lib/bspatch.ts`** (rewritten): - `Bun.zstdDecompressSync()` → `zstdDecompressSync()` from `node:zlib` - `DecompressionStream('zstd')` → `createZstdDecompress()` from `node:zlib` piped through a Node Readable→Web ReadableStream bridge - `Bun.mmap()` → removed entirely; uses `readFile()` for both paths (copy-then-read and direct-read fallback) - `Bun.CryptoHasher("sha256")` → `createHash("sha256")` from `node:crypto` - `Bun.file(destPath).writer()` → `createWriteStream(destPath)` from `node:fs` **`src/lib/upgrade.ts`**: - `Bun.file(destPath).writer()` → `createWriteStream(destPath)` from `node:fs` **`src/lib/api/sourcemaps.ts`**: - `Bun.zstdCompress(buf, { level: 3 })` → `zstdCompress()` from `node:zlib` **`src/lib/telemetry/zstd-transport.ts`**: - Removed `globalThis.Bun.zstdCompress` dynamic access and `BunZstdHost` type - Replaced with direct `zstdCompress` from `node:zlib` (always available in Node 22.15+) - Removed belt-and-braces fallback (zstd can't disappear at runtime with `node:zlib`) - `hasZstdSupport()` now checks `typeof zstdCompressCb === "function"` directly **Tests updated** (`test/lib/telemetry/zstd-transport.test.ts`): - Removed 4 tests for `globalThis.Bun.zstdCompress` fallback paths (no longer applicable) - Replaced `Bun.zstdDecompress` with `zstdDecompressSync` in assertions ### Result **Zero non-comment `Bun.*` API calls remain in `src/`.** The only `bun:` reference left is the `require("bun:sqlite")` fallback in `sqlite.ts`, which will be removed when the test runner migrates to Vitest. All 7014 tests pass, 0 failures.
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
## Summary Consolidates fixes for review comments across PRs #967, #970, #984, #986, superseding #988. ### Critical - **Bump `engines.node` to `>=22.15`** — `node:zlib` zstd APIs require 22.15+. Node 20 is EOL. Updated `dist/bin.cjs` runtime version check to match. - **Simplify zstd imports** — replaced feature-detection dance with direct `import { zstdCompress } from "node:zlib"` now that >=22.15 is guaranteed. ### High - **Fix spawn error handling in `upgrade.ts`** — `proc.on("error", () => resolve(1))` discarded the error object, making ENOENT/EBUSY detection dead code. Now properly rejects with the error. ### Medium - **Fix `sqlite.ts` ROLLBACK** — if ROLLBACK throws, the original transaction error was lost. Now wrapped in try/catch. - **Guard `semver.compare`** in `delta-upgrade.ts` with `semver.valid()` to avoid throwing on invalid version strings. - **Narrow catch in `shell.ts`** `addToGitHubPath` — only catch ENOENT, re-throw EACCES/EPERM. - **Add WriteStream backpressure** in `upgrade.ts` `streamDecompressToFile` — check `write()` return value, await `'drain'`. - **Add `setup-node@v6`** with Node 22 to E2E CI job. ### Low - **Fix `whichSync` Windows CRLF** — split on `/\r?\n/` instead of `\n` to strip trailing `\r` from `where.exe` output. ### Test results All 7014 tests pass, 0 failures. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>


Summary
First step of the Bun → Node.js migration. Switches the project's package manager from bun to pnpm while keeping bun as the TS runner and test runner for now (incremental migration).
Changes
packageManagertopnpm@10.11.0patchedDependenciesintopnpmconfig section (pnpm nests it under"pnpm": {})onlyBuiltDependencies: ["esbuild"]for pnpm's build script approval@sentry/coreand@clack/promptsas explicitdevDependencies— these were phantom deps hoisted by bun's flatnode_modulesbut imported directly by source codepnpm-lock.yamlTest results
All 6968 unit tests pass under
bun testwith pnpm'snode_modules. One pre-existing test fragility (sdk.run throws when auth is required but missingintest/lib/index.test.ts) surfaces under pnpm's strict hoisting — the mock fetch returns empty 200s which prevents the expected auth error. This is unrelated to this change and will be addressed separately.Notes
bun.lockis intentionally kept for now — will be removed in a cleanup PR after the full migration@stricli/core,@sentry/core,@sentry/node-core) apply successfully under pnpmbun run/bun testinternally — migrating those totsx/vitestis a separate step