fix(install): respect deprecated field in npm package manifests (#25314) #25317
Conversation
…-sh#25314) - Add deprecated field to PackageVersion struct - Parse deprecated field from package manifests - Filter deprecated versions in findBestVersion, searchVersionList, and findByDistTagWithFilter - Fallback to deprecated versions only when no non-deprecated alternatives exist Fixes oven-sh#25314
WalkthroughAdds a Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
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 |
|
@samuelcastro please review my solution. |
|
The cache serializer version at line 938 needs to be bumped: // Current:
pub const version = "bun-npm-manifest-cache-v0.0.7\n";
// Should be:
pub const version = "bun-npm-manifest-cache-v0.0.8\n";
Also add a changelog comment:
// - v0.0.8: added deprecated field for filtering out deprecated package versionsWithout this change, existing cached manifests will have deprecated = false for ALL packages (including actually deprecated ones) because the old cache format doesn't include this field. Users will experience inconsistent behavior where the fix only works after clearing their cache or after cache entries expire. |
|
Also consider adding a test case that mirrors the original issue - where stable releases (1.0.0-1.0.3) are deprecated but a prerelease (1.0.0-beta.5) is not, and verify the prerelease gets selected for a ^1.0.0 range. |
- Bump cache serializer version from v0.0.7 to v0.0.8 - Add changelog comment for deprecated field - Update test to use semver range (^1.0.0) instead of exact version - Better mirrors original issue where deprecated stable releases should be skipped in favor of non-deprecated prereleases
|
Hi @samuelcastro, Thank you for the detailed feedback! I've addressed both of your points: 1. Cache Serializer Version Bump ✅Updated the cache serializer version from // - v0.0.8: added deprecated field for filtering out deprecated package versions
pub const version = "bun-npm-manifest-cache-v0.0.8\n";This ensures that existing cached manifests will be invalidated and re-fetched with the 2. Enhanced Test Case ✅Updated dependencies: {
// Use semver range to mirror the original issue:
// Stable releases 1.0.0-1.0.3 are deprecated, but prerelease 1.0.0-beta.5 is not
// ^1.0.0 should select the non-deprecated prerelease over deprecated stable releases
"@mastra/deployer": "^1.0.0",
}This verifies that when using a semver range, Bun correctly selects the non-deprecated prerelease version (1.0.0-beta.5) over the deprecated stable releases (1.0.0-1.0.3). Both changes are now ready for review. Please let me know if you need any additional modifications! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/install/npm.zig(10 hunks)test/cli/install/bun-install-deprecated.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/cli/install/bun-install-deprecated.test.ts
test/cli/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
When testing Bun as a CLI, use the
spawnAPI frombunwith thebunExe()andbunEnvfromharnessto execute Bun commands and validate exit codes, stdout, and stderr
Files:
test/cli/install/bun-install-deprecated.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/cli/install/bun-install-deprecated.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using-eflag overtempDir
For multi-file tests in Bun test suite, prefer usingtempDirandBun.spawn
Always useport: 0when spawning servers in tests - do not hardcode ports or use custom random port functions
UsenormalizeBunSnapshotto normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
UsetempDirfromharnessto create temporary directories in tests - do not usetmpdirSyncorfs.mkdtempSync
In tests, callexpect(stdout).toBe(...)beforeexpect(exitCode).toBe(0)when spawning processes for more useful error messages on failure
Do not write flaky tests - do not usesetTimeoutin tests; insteadawaitthe condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- tests are not valid if they pass withUSE_SYSTEM_BUN=1
Avoid shell commands in tests - do not usefindorgrep; use Bun's Glob and built-in tools instead
Test files must end in.test.tsor.test.tsxand be created in the appropriate test folder structure
Files:
test/cli/install/bun-install-deprecated.test.ts
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/install/npm.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)Be careful with memory management in Zig code - use defer for cleanup with allocators
Files:
src/install/npm.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/install/npm.zig
🧠 Learnings (15)
📓 Common learnings
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-vscode/package.json:382-385
Timestamp: 2025-11-20T19:51:32.288Z
Learning: In the Bun repository, dependencies may be explicitly added to package.json files (even when not directly imported in code) to force version upgrades on transitive dependencies, particularly as part of Aikido security scanner remediation to ensure vulnerable transitive dependencies resolve to patched versions.
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - tests are not valid if they pass with `USE_SYSTEM_BUN=1`
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/bundle.test.ts : Organize bundle tests in bundle.test.ts for tests concerning bundling bugs that only occur in DevServer
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : For multi-file tests in Bun test suite, prefer using `tempDir` and `Bun.spawn`
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For test/js/node/test/{parallel,sequential}/*.js files without a .test extension, use `bun bd <file>` instead of `bun bd test <file>` since these expect exit code 0 and don't use bun's test runner
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-20T19:51:32.288Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-vscode/package.json:382-385
Timestamp: 2025-11-20T19:51:32.288Z
Learning: In the Bun repository, dependencies may be explicitly added to package.json files (even when not directly imported in code) to force version upgrades on transitive dependencies, particularly as part of Aikido security scanner remediation to ensure vulnerable transitive dependencies resolve to patched versions.
Applied to files:
test/cli/install/bun-install-deprecated.test.tssrc/install/npm.zig
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/cli/install/bun-install-deprecated.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/install/npm.zig
🧬 Code graph analysis (1)
test/cli/install/bun-install-deprecated.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
🔇 Additional comments (2)
src/install/npm.zig (2)
875-876: LGTM!The
deprecatedfield is correctly added to thePackageVersionstruct with a sensible default offalse. The size check is properly updated to reflect the new struct size.Also applies to: 884-884
941-942: LGTM!The cache serializer version is correctly bumped to v0.0.8 with an appropriate changelog comment explaining the addition of the deprecated field. This matches the PR requirements.
| // Skip deprecated versions | ||
| if (package.deprecated) continue; |
There was a problem hiding this comment.
Missing fallback mechanism for deprecated versions.
Unlike searchVersionList (lines 1505-1596) and findBestVersion (lines 1830-1893), the findByDistTagWithFilter function skips deprecated versions without tracking a fallback. If all versions matching the dist tag filter are deprecated, this will return an error instead of falling back to the best deprecated version.
This inconsistency could cause installation failures when a package's dist tag (e.g., "latest") points to a deprecated version and all recent non-deprecated versions are filtered out by age constraints.
Consider adding a fallback mechanism similar to the other functions:
var best_version: ?FindResult = null;
var prev_package_blocked_from_age: ?*const PackageVersion = dist_result.package;
+ var fallback_deprecated: ?FindResult = null;
var i: usize = versions.len;
while (i > 0) : (i -= 1) {
const idx = i - 1;
const version = versions[idx];
const package = &packages[idx];
if (version.order(latest_version, this.string_buf, this.string_buf) == .gt) continue;
if (latest_version_tag_before_dot) |expected_tag| {
const package_tag = version.tag.pre.slice(this.string_buf);
const actual_tag =
if (strings.indexOfChar(package_tag, '.')) |dot_i| package_tag[0..dot_i] else package_tag;
if (!strings.eql(actual_tag, expected_tag)) continue;
}
- // Skip deprecated versions
- if (package.deprecated) continue;
+ // Skip deprecated versions, but keep track as fallback
+ if (package.deprecated) {
+ if (fallback_deprecated == null) {
+ fallback_deprecated = .{
+ .version = version,
+ .package = package,
+ };
+ }
+ continue;
+ }
// ... rest of the function
}
if (best_version) |result| {
return .{ .found_with_filter = .{
.result = result,
.newest_filtered = dist_result.version,
} };
}
+
+ // If we only found deprecated versions, return the best deprecated one
+ if (fallback_deprecated) |result| {
+ return .{ .found_with_filter = .{
+ .result = result,
+ .newest_filtered = dist_result.version,
+ } };
+ }
return .{ .err = .all_versions_too_recent };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/install/npm.zig around lines 1680-1681, the function currently skips
deprecated versions outright causing no fallback when all matching versions are
deprecated; modify the loop to track the best deprecated candidate (store its
version and metadata when encountering a deprecated entry that passes the
dist-tag filter) while still preferring non-deprecated versions, and after the
loop, if no non-deprecated match was chosen but a deprecated candidate exists,
return that deprecated candidate instead of failing; ensure selection logic
(semver comparison/age checks) used for non-deprecated choices is reused for
choosing the best deprecated fallback and keep existing error paths unchanged.
| // Parse deprecated field | ||
| if (prop.value.?.asProperty("deprecated")) |_| { | ||
| // If the deprecated field exists (regardless of its value), mark as deprecated | ||
| // npm marks packages as deprecated with either a string message or boolean true | ||
| package_version.deprecated = true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating the deprecated field value.
The current implementation marks a version as deprecated whenever the deprecated field exists, regardless of its value. While npm typically only uses string messages or true, explicitly checking the value would be more robust and semantically correct.
Apply this diff to validate the field value:
// Parse deprecated field
- if (prop.value.?.asProperty("deprecated")) |_| {
- // If the deprecated field exists (regardless of its value), mark as deprecated
- // npm marks packages as deprecated with either a string message or boolean true
- package_version.deprecated = true;
+ if (prop.value.?.asProperty("deprecated")) |deprecated_prop| {
+ // npm marks packages as deprecated with either a string message or boolean true
+ switch (deprecated_prop.expr.data) {
+ .e_string => |str| {
+ package_version.deprecated = str.data.len > 0;
+ },
+ .e_boolean => |boolean| {
+ package_version.deprecated = boolean.value;
+ },
+ else => {
+ // Treat any other value as deprecated for safety
+ package_version.deprecated = true;
+ },
+ }
}This would correctly handle the edge case where deprecated: false is explicitly set (though npm doesn't do this in practice).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/install/npm.zig around lines 2617 to 2622, the code marks
package_version.deprecated = true whenever the deprecated field exists
regardless of value; change it to validate the field's value: only set
deprecated = true when the field is the boolean true or a non-empty string
(treat boolean false or empty string as not deprecated), preserving current
behavior for other types by ignoring them; implement the check by examining
prop.value's runtime type and content and assign package_version.deprecated
accordingly.
| import { join } from "path"; | ||
| import { mkdir, writeFile, rm } from "fs/promises"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused imports.
The join and mkdir, writeFile, rm imports will be unused if you adopt the tempDir() refactor suggested above.
🤖 Prompt for AI Agents
In test/cli/install/bun-install-deprecated.test.ts around lines 3 to 4, remove
the unused imports (join from "path" and mkdir, writeFile, rm from
"fs/promises") because the tempDir() refactor makes them unnecessary; update the
import statements to only include the modules still used in this file (delete
the unused names) and run tests/linter to ensure no remaining unused-import
errors.
| test("bun install respects deprecated field", async () => { | ||
| const cwd = join(import.meta.dir, "bun-install-deprecated-" + Date.now()); | ||
| await mkdir(cwd, { recursive: true }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use tempDir() from harness instead of manual directory management.
Per coding guidelines, use tempDir() from harness to create temporary directories for tests instead of manually creating and cleaning up directories.
Apply this diff to use tempDir():
+import { tempDirWithFiles } from "harness";
+
test("bun install respects deprecated field", async () => {
- const cwd = join(import.meta.dir, "bun-install-deprecated-" + Date.now());
- await mkdir(cwd, { recursive: true });
+ const cwd = tempDirWithFiles("bun-install-deprecated", {
+ "package.json": JSON.stringify({
+ dependencies: {
+ // Use semver range to mirror the original issue:
+ // Stable releases 1.0.0-1.0.3 are deprecated, but prerelease 1.0.0-beta.5 is not
+ // ^1.0.0 should select the non-deprecated prerelease over deprecated stable releases
+ "@mastra/deployer": "^1.0.0",
+ },
+ }),
+ });
-
- await writeFile(
- join(cwd, "package.json"),
- JSON.stringify({
- dependencies: {
- // Use semver range to mirror the original issue:
- // Stable releases 1.0.0-1.0.3 are deprecated, but prerelease 1.0.0-beta.5 is not
- // ^1.0.0 should select the non-deprecated prerelease over deprecated stable releases
- "@mastra/deployer": "^1.0.0",
- },
- })
- );Then remove the cleanup at line 45 since tempDir() handles it automatically.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/cli/install/bun-install-deprecated.test.ts around lines 6 to 8, the test
currently creates a temp directory manually with join(...) and mkdir; replace
that manual directory management with harness.tempDir() to create the test
working directory (assign to cwd) and remove the mkdir call; update any imports
to include tempDir from the harness module if missing; also remove the manual
cleanup at line 45 because tempDir() automatically handles cleanup.
| const { stdout, stderr, exitCode } = Bun.spawnSync({ | ||
| cmd: [bunExe(), "install"], | ||
| cwd, | ||
| env: bunEnv, | ||
| }); | ||
|
|
||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Check stdout/stderr before exitCode for better error messages.
Per coding guidelines, when spawning processes in tests, call expect(stdout).toBe(...) or examine output before checking expect(exitCode).toBe(0) to get more useful error messages on failure.
Apply this diff:
const { stdout, stderr, exitCode } = Bun.spawnSync({
cmd: [bunExe(), "install"],
cwd,
env: bunEnv,
});
+ if (exitCode !== 0) {
+ console.error("Install failed:", stderr.toString());
+ }
expect(exitCode).toBe(0);🤖 Prompt for AI Agents
In test/cli/install/bun-install-deprecated.test.ts around lines 22 to 28, the
test asserts exitCode before inspecting process output which hides useful
failure details; modify the test to assert or at least inspect stdout and stderr
(e.g., expect(stderr).toBe('') and/or expect(stdout).toContain(...)) before
calling expect(exitCode).toBe(0) so that on failure the test runner shows the
process output first and gives better error messages.
| // We can't strictly assert 1.0.0-beta.5 because newer versions might be released, | ||
| // but we know 1.0.3 is the problematic deprecated one. | ||
| // However, based on the issue, 1.0.0-beta.5 is the expected one for that specific deployer version. | ||
| expect(serverPkgJson.version).toBe("1.0.0-beta.5"); |
There was a problem hiding this comment.
Remove contradictory comment or make assertion conditional.
The comment on lines 40-42 says "We can't strictly assert 1.0.0-beta.5 because newer versions might be released", but line 43 does exactly that strict assertion. This is contradictory and will cause the test to fail if a newer non-deprecated version is published.
Either:
- Remove the comment and keep the strict assertion (if you want the test to break when new versions are published), or
- Make the assertion conditional as the comment suggests:
- // Should be 1.0.0-beta.5 or similar non-deprecated version
- // We can't strictly assert 1.0.0-beta.5 because newer versions might be released,
- // but we know 1.0.3 is the problematic deprecated one.
- // However, based on the issue, 1.0.0-beta.5 is the expected one for that specific deployer version.
- expect(serverPkgJson.version).toBe("1.0.0-beta.5");
+ // Should be a non-deprecated version (1.0.0-beta.5 or newer)
+ // We know 1.0.3 is deprecated and should not be selected
+ expect(serverPkgJson.version).toMatch(/^1\.0\.0-beta\.\d+$/);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/cli/install/bun-install-deprecated.test.ts around lines 40-43, the
comment claims we "can't strictly assert 1.0.0-beta.5" but the next line asserts
that exact version, which is contradictory and brittle; either remove the
comment and keep the strict assertion, or make the test conditional: update the
assertion to allow newer non-deprecated versions (e.g., use a semver check or
string prefix match) or gate the exact-equals assertion behind a conditional so
the test only enforces "1.0.0-beta.5" when appropriate; pick one approach and
update the comment to reflect the chosen behavior.
2 participants
This PR fixes issue #25314 where Bun would install deprecated package versions (e.g., marked with "bad publish") even when non-deprecated alternatives were available.
Changes
deprecatedfield toPackageVersionstruct: Updated the struct size to 248 bytes to accommodate the new boolean field.deprecatedfield: Updated the manifest parsing logic insrc/install/npm.zigto read thedeprecatedfield from package versions.findBestVersion,searchVersionList,findByDistTagWithFilter) to skip deprecated versions.Verification
test/cli/install/bun-install-deprecated.test.ts.bun installnow selects the non-deprecated version (e.g.,1.0.0-beta.5) instead of the deprecated one (1.0.3) for the reported issue case.Fixes #25314