Skip to content

fix(install): respect deprecated field in npm package manifests (#25314) #25317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Advikkhandelwal wants to merge 2 commits into
base: main
Choose a base branch
from
+128 −10
Open

fix(install): respect deprecated field in npm package manifests (#25314) #25317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b27397
fix(install): respect deprecated field in npm package manifests (#25314)
Dec 3, 2025
1a7d68f
Address maintainer feedback: bump cache version and enhance test
Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 82 additions & 10 deletions src/install/npm.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,13 +872,16 @@ pub const PackageVersion = extern struct {
/// Unix timestamp when this version was published (0 if unknown)
publish_timestamp_ms: f64 = 0,

/// Whether this version is marked as deprecated in the npm registry
deprecated: bool = false,

pub fn allDependenciesBundled(this: *const PackageVersion) bool {
return this.bundled_dependencies.isInvalid();
}
};

comptime {
if (@sizeOf(Npm.PackageVersion) != 240) {
if (@sizeOf(Npm.PackageVersion) != 248) {
@compileError(std.fmt.comptimePrint("Npm.PackageVersion has unexpected size {d}", .{@sizeOf(Npm.PackageVersion)}));
}
}
Expand Down Expand Up @@ -935,7 +938,8 @@ pub const PackageManifest = struct {
// - v0.0.5: added bundled dependencies
// - v0.0.6: changed semver major/minor/patch to each use u64 instead of u32
// - v0.0.7: added version publish times and extended manifest flag for minimum release age
pub const version = "bun-npm-manifest-cache-v0.0.7\n";
// - v0.0.8: added deprecated field for filtering out deprecated package versions
pub const version = "bun-npm-manifest-cache-v0.0.8\n";
const header_bytes: string = "#!/usr/bin/env bun\n" ++ version;

pub const sizes = blk: {
Expand Down Expand Up @@ -1498,6 +1502,7 @@ pub const PackageManifest = struct {
) ?FindVersionResult {
var prev_package_blocked_from_age: ?*const PackageVersion = null;
var best_version: ?FindResult = null;
var fallback_deprecated: ?FindResult = null;

const current_timestamp_ms: f64 = @floatFromInt(@divTrunc(bun.start_time, std.time.ns_per_ms));
const seven_days_ms: f64 = 7 * std.time.ms_per_day;
Expand All @@ -1509,6 +1514,18 @@ pub const PackageManifest = struct {
const version = versions[i];
if (group.satisfies(version, group_buf, this.string_buf)) {
const package = &packages[i];

// Skip deprecated versions, but keep track as fallback
if (package.deprecated) {
if (fallback_deprecated == null) {
fallback_deprecated = .{
.version = version,
.package = package,
};
}
continue;
}

if (isPackageVersionTooRecent(package, minimum_release_age_ms)) {
if (newest_filtered.* == null) newest_filtered.* = version;
prev_package_blocked_from_age = package;
Expand Down Expand Up @@ -1564,6 +1581,19 @@ pub const PackageManifest = struct {
return .{ .found = result };
}
}

// If we only found deprecated versions, return the best deprecated one
if (fallback_deprecated) |result| {
if (newest_filtered.*) |nf| {
return .{ .found_with_filter = .{
.result = result,
.newest_filtered = nf,
} };
} else {
return .{ .found = result };
}
}

return null;
}

Expand Down Expand Up @@ -1647,6 +1677,9 @@ pub const PackageManifest = struct {
if (!strings.eql(actual_tag, expected_tag)) continue;
}

// Skip deprecated versions
if (package.deprecated) continue;
Comment on lines +1680 to +1681
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


if (isPackageVersionTooRecent(package, min_age_ms)) {
prev_package_blocked_from_age = package;
continue;
Expand Down Expand Up @@ -1780,17 +1813,22 @@ pub const PackageManifest = struct {

if (this.findByDistTag("latest")) |result| {
if (group.satisfies(result.version, group_buf, this.string_buf)) {
if (group.flags.isSet(Semver.Query.Group.Flags.pre)) {
if (left.version.order(result.version, group_buf, this.string_buf) == .eq) {
// if prerelease, use latest if semver+tag match range exactly
// Skip deprecated latest version if possible
if (!result.package.deprecated) {
if (group.flags.isSet(Semver.Query.Group.Flags.pre)) {
if (left.version.order(result.version, group_buf, this.string_buf) == .eq) {
// if prerelease, use latest if semver+tag match range exactly
return result;
}
} else {
return result;
}
} else {
return result;
}
}
}

var fallback_deprecated: ?FindResult = null;

{
// This list is sorted at serialization time.
const releases = this.pkg.releases.keys.get(this.versions);
Expand All @@ -1800,9 +1838,22 @@ pub const PackageManifest = struct {
const version = releases[i - 1];

if (group.satisfies(version, group_buf, this.string_buf)) {
const package = &this.pkg.releases.values.get(this.package_versions)[i - 1];

// Skip deprecated versions, but keep track of the first one as fallback
if (package.deprecated) {
if (fallback_deprecated == null) {
fallback_deprecated = .{
.version = version,
.package = package,
};
}
continue;
}

return .{
.version = version,
.package = &this.pkg.releases.values.get(this.package_versions)[i - 1],
.package = package,
};
}
}
Expand All @@ -1817,15 +1868,29 @@ pub const PackageManifest = struct {
// This list is sorted at serialization time.
if (group.satisfies(version, group_buf, this.string_buf)) {
const packages = this.pkg.prereleases.values.get(this.package_versions);
const package = &packages[i - 1];

// Skip deprecated versions, but keep track of the first one as fallback
if (package.deprecated) {
if (fallback_deprecated == null) {
fallback_deprecated = .{
.version = version,
.package = package,
};
}
continue;
}

return .{
.version = version,
.package = &packages[i - 1],
.package = package,
};
}
}
}

return null;
// If we only found deprecated versions, return the best deprecated one
return fallback_deprecated;
}

const ExternalStringMapDeduper = std.HashMap(u64, ExternalStringList, IdentityContext(u64), 80);
Expand Down Expand Up @@ -2549,6 +2614,13 @@ pub const PackageManifest = struct {
}
}

// 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;
}
Comment on lines +2617 to +2622
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


if (!parsed_version.version.tag.hasPre()) {
release_versions[0] = parsed_version.version.min();
versioned_package_releases[0] = package_version;
Expand Down
46 changes: 46 additions & 0 deletions test/cli/install/bun-install-deprecated.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { test, expect } from "bun:test";
import { bunExe, bunEnv } from "harness";
import { join } from "path";
import { mkdir, writeFile, rm } from "fs/promises";
Comment on lines +3 to +4
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 });
Comment on lines +6 to +8
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


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",
},
})
);

const { stdout, stderr, exitCode } = Bun.spawnSync({
cmd: [bunExe(), "install"],
cwd,
env: bunEnv,
});

expect(exitCode).toBe(0);
Comment on lines +22 to +28
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


// Check installed version of @mastra/server
const serverPkgJsonPath = join(cwd, "node_modules", "@mastra", "server", "package.json");
const serverPkgJson = await Bun.file(serverPkgJsonPath).json();

console.log("Installed @mastra/server version:", serverPkgJson.version);

// Should NOT be 1.0.3 (which is deprecated)
expect(serverPkgJson.version).not.toBe("1.0.3");

// 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");
Comment on lines +40 to +43
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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:

  1. Remove the comment and keep the strict assertion (if you want the test to break when new versions are published), or
  2. 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.


await rm(cwd, { recursive: true, force: true });
}, 60000); // Increase timeout for network operations