Fix hosted yarn classic pins missing berry warning (#907)

crates/socket-patch-bench/src/fixtures/npm.rs

@@ -537,7 +537,17 @@ pub fn yarn_classic_lock(g: &Graph) -> String {
 
 pub fn build_yarn_classic(t: &mut Tree, size: Size) -> std::io::Result<Fixture> {
     let g = graph("yarn-classic", size);
-    t.write("project/package.json", g.package_json())?;
+    // Pin yarn 1 as a real classic project does: without the pin a hosted
+    // scan rightly warns `redirect_yarn_classic_berry_migration_risk`
+    // (#907), which the scenario would count as an unexpected warning.
+    t.write(
+        "project/package.json",
+        g.package_json().replacen(
+            "\"private\": true",
+            "\"private\": true,\n  \"packageManager\": \"yarn@1.22.22\"",
+            1,
+        ),
+    )?;
     t.write("project/yarn.lock", yarn_classic_lock(&g))?;
     t.write(
         "project/node_modules/.yarn-integrity",

crates/socket-patch-cli/tests/covgap_commands_scan_hosted.rs

@@ -2607,3 +2607,105 @@ async fn vlt_decides_before_binary_bun_and_a_refused_uuid_is_never_confirmed() {
     let (confirmed, _tmp) = run(false);
     assert_eq!(confirmed["redirect"]["redirected"], 1, "{confirmed:#}");
 }
+
+// ───────────────── yarn classic berry-migration advisory ─────────────────
+
+/// Yarn classic project: package.json (with `package_manager` when given) +
+/// installed node_modules copy + a v1 yarn.lock resolving NAME upstream.
+fn write_yarn_classic_project(root: &Path, package_manager: Option<&str>) {
+    let pm = package_manager
+        .map(|pm| format!(r#", "packageManager": "{pm}""#))
+        .unwrap_or_default();
+    std::fs::write(
+        root.join("package.json"),
+        format!(
+            r#"{{ "name": "consumer", "version": "0.0.0"{pm}, "dependencies": {{ "{NAME}": "{VERSION}" }} }}"#
+        ),
+    )
+    .unwrap();
+    let pkg = root.join("node_modules").join(NAME);
+    std::fs::create_dir_all(&pkg).unwrap();
+    std::fs::write(
+        pkg.join("package.json"),
+        format!(r#"{{ "name": "{NAME}", "version": "{VERSION}" }}"#),
+    )
+    .unwrap();
+    std::fs::write(
+        root.join("yarn.lock"),
+        format!(
+            "# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.\n\
+             # yarn lockfile v1\n\n\n\
+             {NAME}@{VERSION}:\n  version \"{VERSION}\"\n  \
+             resolved \"https://registry.yarnpkg.com/{NAME}/-/{NAME}-{VERSION}.tgz#bbbb\"\n  \
+             integrity {UPSTREAM_SHA512}\n"
+        ),
+    )
+    .unwrap();
+}
+
+/// #907: a hosted pin in a classic yarn.lock is dropped by the next yarn 2+
+/// (berry) install exactly like vendored wiring, so `scan --mode hosted`
+/// must warn `redirect_yarn_classic_berry_migration_risk` — on a dry run, on
+/// the wet run and on an idempotent re-run — when package.json declares no
+/// package manager or a berry one.
+#[tokio::test]
+async fn hosted_yarn_classic_pin_warns_berry_migration_risk() {
+    for package_manager in [None, Some("yarn@4.18.1")] {
+        let server = MockServer::start().await;
+        mock_discovery(&server, PURL, UUID).await;
+        mock_granted_reference(&server, UUID, PURL, HOSTED_URL).await;
+        mock_view(&server, UUID, PURL).await;
+        let tmp = tempfile::tempdir().unwrap();
+        write_yarn_classic_project(tmp.path(), package_manager);
+
+        for (label, extra) in [
+            ("dry run", &["--dry-run"][..]),
+            ("wet run", &[][..]),
+            ("re-run", &[][..]),
+        ] {
+            let (code, doc) = scan_hosted_json(tmp.path(), &server.uri(), extra, &[]);
+            assert_eq!(code, 0, "{package_manager:?} {label}: {doc:#}");
+            let codes = warning_codes(&doc);
+            assert_eq!(
+                codes
+                    .iter()
+                    .filter(|c| *c == "redirect_yarn_classic_berry_migration_risk")
+                    .count(),
+                1,
+                "{package_manager:?} {label}: exactly one advisory: {codes:?}"
+            );
+            let detail = warning_detail(&doc, "redirect_yarn_classic_berry_migration_risk");
+            assert!(detail.contains("yarn 2+"), "{detail}");
+        }
+        let lock = std::fs::read_to_string(tmp.path().join("yarn.lock")).unwrap();
+        assert!(
+            lock.contains(HOSTED_URL),
+            "the warning never blocks the pin: {lock}"
+        );
+    }
+}
+
+/// #907: a corepack `packageManager: yarn@1…` pin makes a stray berry
+/// install refuse instead of migrate, so hosted stays silent, as vendored
+/// does.
+#[tokio::test]
+async fn hosted_yarn_classic_pin_with_yarn1_package_manager_stays_silent() {
+    let server = MockServer::start().await;
+    mock_discovery(&server, PURL, UUID).await;
+    mock_granted_reference(&server, UUID, PURL, HOSTED_URL).await;
+    mock_view(&server, UUID, PURL).await;
+    let tmp = tempfile::tempdir().unwrap();
+    write_yarn_classic_project(tmp.path(), Some("yarn@1.22.22"));
+
+    let (code, doc) = scan_hosted_json(tmp.path(), &server.uri(), &[], &[]);
+    assert_eq!(code, 0, "{doc:#}");
+    let codes = warning_codes(&doc);
+    assert!(
+        !codes
+            .iter()
+            .any(|c| c == "redirect_yarn_classic_berry_migration_risk"),
+        "a yarn 1 pin suppresses the advisory: {codes:?}"
+    );
+    let lock = std::fs::read_to_string(tmp.path().join("yarn.lock")).unwrap();
+    assert!(lock.contains(HOSTED_URL), "{lock}");
+}

crates/socket-patch-core/src/hosted/engine.rs

@@ -468,12 +468,16 @@ pub async fn read_candidate_files(
     {
         out.read(view, unreadable, "package.json").await;
     // Otherwise the root manifest's `overrides` decide which git / url /
-    // `file:` dependent specs npm really installs from (#490). Only the npm
-    // lock rewriter reads it, as advisory input: no rewriter edits it, so a
-    // link or an unreadable in-memory entry is left out (the rewriter then
-    // keeps its conservative reading) rather than refused.
+    // `file:` dependent specs npm really installs from (#490), and beside a
+    // classic `yarn.lock` its `packageManager` says whether a yarn 2+ install
+    // could migrate the lock and drop the hosted pins (#907). The npm lock
+    // and yarn classic rewriters read it as advisory input only: no
+    // rewriter edits it, so a link or an unreadable in-memory entry is left
+    // out (the rewriters then keep their conservative reading) rather than
+    // refused.
     } else if candidates.iter().any(|c| c.dep.ecosystem == "npm")
-        && NPM_LOCKS.iter().any(|lock| out.files.contains_key(*lock))
+        && (NPM_LOCKS.iter().any(|lock| out.files.contains_key(*lock))
+            || out.files.contains_key("yarn.lock"))
     {
         let rel = crate::hosted::memory::select::NPM_MANIFEST_REL;
         if let Some(text) = read_advisory(view, unreadable, rel).await {

crates/socket-patch-core/src/patch/redirect/mod.rs

@@ -3505,6 +3505,7 @@ fn rewrite_yarn_classic(
     let mut heads: Vec<Option<(String, Option<String>)>> =
         blocks.iter().map(|b| yarn_classic_block_head(b)).collect();
     let mut changed = false;
+    let mut any_pinned = false;
     for dep in &npm {
         let fname = full_name(dep);
         let Some(sha512) = dep.integrity.sha512.clone() else {
@@ -3663,6 +3664,26 @@ fn rewrite_yarn_classic(
                 detail: format!("no yarn.lock entry resolving {fname}@{}", dep.version),
             });
         }
+        any_pinned |= matched_any;
+    }
+    // Yarn 2+ (berry) migrates a classic lock on install and re-resolves
+    // every entry from the registry, dropping the hosted pins this lock now
+    // carries (rewritten this run or already pinned) — the packages then
+    // install unpatched with nothing printed. The vendored probe
+    // (`vendor::yarn_classic_berry_migration_risk`) warns about the same
+    // trap; warn here too, once per run, unless corepack pins yarn 1. The
+    // engine reads the root manifest beside a classic lock; with none there
+    // is no project for yarn to install, so nothing to warn about.
+    let manifest = files.get("package.json").map(String::as_str);
+    if any_pinned && manifest.is_some() && !crate::vendor::manifest_pins_yarn_classic(manifest) {
+        result.warnings.push(RewriteWarning {
+            code: "redirect_yarn_classic_berry_migration_risk".into(),
+            detail: "yarn.lock is yarn-classic (v1) with hosted pins: installing with yarn 2+ \
+                     (berry) migrates the lockfile and silently drops them — packages install \
+                     unpatched from the registry. Pin yarn classic (e.g. \"packageManager\": \
+                     \"yarn@1.22.22\" in package.json) so every install uses yarn 1."
+                .into(),
+        });
     }
     if let Some(warning) = mirror_refusal {
         if !result.refused_yarn_classic_uuids.is_empty() {
@@ -9956,6 +9977,158 @@ mod tests {
             .to_string()
     }
 
+    const BERRY_RISK: &str = "redirect_yarn_classic_berry_migration_risk";
+
+    fn classic_files(package_json: Option<&str>) -> BTreeMap<String, String> {
+        let mut files = BTreeMap::new();
+        files.insert("yarn.lock".to_string(), classic_lock_two_entries());
+        if let Some(manifest) = package_json {
+            files.insert("package.json".to_string(), manifest.to_string());
+        }
+        files
+    }
+
+    fn berry_risk_count(r: &RewriteResult) -> usize {
+        r.warnings.iter().filter(|w| w.code == BERRY_RISK).count()
+    }
+
+    /// #907: a hosted pin in a classic lock is dropped by a yarn 2+ install
+    /// exactly like vendored wiring, so the hosted rewrite must warn the
+    /// way the vendored probe does — with no `packageManager` pin, with a
+    /// non-1 yarn declared (`yarn@10` must not prefix-match `yarn@1`), and
+    /// when the manifest is unparseable (fail toward warning).
+    #[test]
+    fn yarn_classic_hosted_pin_warns_berry_migration_risk() {
+        let ovr = npm_override(
+            "left-pad",
+            "1.3.0",
+            "http://p.test/lp.tgz",
+            "sha512-PATCHED==",
+        );
+        for manifest in [
+            Some(r#"{"name":"p","dependencies":{"left-pad":"^1.3.0"}}"#),
+            Some(r#"{"name":"p","packageManager":"yarn@4.18.1"}"#),
+            Some(r#"{"name":"p","packageManager":"yarn@10.0.0"}"#),
+            Some(r#"{"name":"p","packageManager":"pnpm@9.0.0"}"#),
+            Some("{not json"),
+        ] {
+            let mut r = RewriteResult::default();
+            rewrite_yarn_classic(&classic_files(manifest), std::slice::from_ref(&ovr), &mut r);
+            assert!(
+                r.files.contains_key("yarn.lock"),
+                "{manifest:?}: pin must land"
+            );
+            assert_eq!(berry_risk_count(&r), 1, "{manifest:?}: {:?}", r.warnings);
+            let w = r.warnings.iter().find(|w| w.code == BERRY_RISK).unwrap();
+            assert!(
+                w.detail.contains("yarn 2+"),
+                "detail names the trap: {}",
+                w.detail
+            );
+            assert!(
+                w.detail.contains("yarn@1"),
+                "detail names the remedy: {}",
+                w.detail
+            );
+        }
+    }
+
+    /// #907: a corepack `packageManager: yarn@1…` pin makes a stray berry
+    /// install refuse instead of migrate, so it suppresses the warning (as
+    /// it does the vendored one), including a pin with a corepack hash.
+    #[test]
+    fn yarn_classic_hosted_pin_yarn1_package_manager_suppresses_berry_risk() {
+        let ovr = npm_override(
+            "left-pad",
+            "1.3.0",
+            "http://p.test/lp.tgz",
+            "sha512-PATCHED==",
+        );
+        for pm in ["yarn@1.22.22", "yarn@1.7.0", "yarn@1.22.22+sha512.abc"] {
+            let manifest = format!(r#"{{"name":"p","packageManager":"{pm}"}}"#);
+            let mut r = RewriteResult::default();
+            rewrite_yarn_classic(
+                &classic_files(Some(&manifest)),
+                std::slice::from_ref(&ovr),
+                &mut r,
+            );
+            assert!(r.files.contains_key("yarn.lock"));
+            assert!(r.warnings.is_empty(), "{pm}: {:?}", r.warnings);
+        }
+    }
+
+    /// #907: the warning is per run, not per package, and it fires on an
+    /// idempotent re-run too: the lock already carries the pin, so the next
+    /// berry install drops it just the same.
+    #[test]
+    fn yarn_classic_berry_risk_is_once_per_run_and_survives_rerun() {
+        let lp = npm_override(
+            "left-pad",
+            "1.3.0",
+            "http://p.test/lp.tgz",
+            "sha512-PATCHED==",
+        );
+        let ab = npm_override("abbrev", "1.1.1", "http://p.test/ab.tgz", "sha512-AB==");
+        let files = classic_files(Some(r#"{"name":"p"}"#));
+        let mut r = RewriteResult::default();
+        rewrite_yarn_classic(&files, &[lp.clone(), ab], &mut r);
+        assert_eq!(r.edits.len(), 2, "both entries pinned: {:?}", r.edits);
+        assert_eq!(berry_risk_count(&r), 1, "{:?}", r.warnings);
+
+        let mut pinned = files.clone();
+        pinned.insert("yarn.lock".into(), r.files["yarn.lock"].clone());
+        let mut again = RewriteResult::default();
+        rewrite_yarn_classic(&pinned, std::slice::from_ref(&lp), &mut again);
+        assert!(
+            again.edits.is_empty(),
+            "re-run is a no-op: {:?}",
+            again.edits
+        );
+        assert_eq!(berry_risk_count(&again), 1, "{:?}", again.warnings);
+    }
+
+    /// No pin in the lock means nothing for berry to drop: a dep with no
+    /// classic entry warns only `entry_not_found`, and a berry lock is not
+    /// the classic rewriter's at all.
+    #[test]
+    fn yarn_classic_berry_risk_silent_without_a_pin() {
+        let missing = npm_override("nope", "9.9.9", "http://p.test/n.tgz", "sha512-N==");
+        let mut r = RewriteResult::default();
+        rewrite_yarn_classic(&classic_files(Some(r#"{"name":"p"}"#)), &[missing], &mut r);
+        let codes: Vec<&str> = r.warnings.iter().map(|w| w.code.as_str()).collect();
+        assert_eq!(codes, ["redirect_yarn_classic_entry_not_found"]);
+
+        // No root manifest: no project for yarn to install, nothing to warn.
+        let lp = npm_override(
+            "left-pad",
+            "1.3.0",
+            "http://p.test/lp.tgz",
+            "sha512-PATCHED==",
+        );
+        let mut r = RewriteResult::default();
+        rewrite_yarn_classic(&classic_files(None), std::slice::from_ref(&lp), &mut r);
+        assert!(r.files.contains_key("yarn.lock"));
+        assert_eq!(berry_risk_count(&r), 0, "{:?}", r.warnings);
+
+        let lp = npm_override(
+            "left-pad",
+            "1.3.0",
+            "http://p.test/lp.tgz",
+            "sha512-PATCHED==",
+        );
+        let mut files = BTreeMap::new();
+        files.insert(
+            "yarn.lock".to_string(),
+            "__metadata:\n  version: 8\n\n\"left-pad@npm:^1.3.0\":\n  version: 1.3.0\n  \
+             resolution: \"left-pad@npm:1.3.0\"\n"
+                .to_string(),
+        );
+        files.insert("package.json".to_string(), r#"{"name":"p"}"#.to_string());
+        let mut r = RewriteResult::default();
+        rewrite_yarn_classic(&files, &[lp], &mut r);
+        assert_eq!(berry_risk_count(&r), 0, "{:?}", r.warnings);
+    }
+
     /// A CRLF classic lock (Windows `core.autocrlf` checkout) must rewrite
     /// the TARGET entry, not whichever entry happens to come first, and every
     /// untouched line must keep its CRLF ending byte-exactly (see the CRLF

crates/socket-patch-core/src/vendor/mod.rs

@@ -184,22 +184,9 @@ pub fn yarn_classic_berry_migration_risk(project_root: &Path) -> Option<VendorWa
     if !lock.contains("# yarn lockfile v1") || !lock.contains(".socket/vendor/") {
         return None;
     }
-    if let Some(pm) = read_regular_to_string_sync(&project_root.join("package.json"))
-        .ok()
-        .and_then(|pkg| serde_json::from_str::<serde_json::Value>(&pkg).ok())
-        .and_then(|v| {
-            v.get("packageManager")
-                .and_then(|p| p.as_str().map(String::from))
-        })
-    {
-        let major = pm.trim().strip_prefix("yarn@").map(|rest| {
-            rest.chars()
-                .take_while(char::is_ascii_digit)
-                .collect::<String>()
-        });
-        if major.as_deref() == Some("1") {
-            return None;
-        }
+    let manifest = read_regular_to_string_sync(&project_root.join("package.json")).ok();
+    if manifest_pins_yarn_classic(manifest.as_deref()) {
+        return None;
     }
     Some(VendorWarning::new(
         "yarn_classic_berry_migration_risk",
@@ -210,6 +197,30 @@ pub fn yarn_classic_berry_migration_risk(project_root: &Path) -> Option<VendorWa
     ))
 }
 
+/// Whether a root `package.json` text pins yarn classic through corepack's
+/// `packageManager: yarn@1…`, which makes a stray yarn 2+ (berry) install
+/// refuse instead of migrating a classic `yarn.lock` and dropping its
+/// pins. A missing, unreadable or malformed manifest vouches for nothing
+/// (`false`), so callers fail toward warning. Shared by the vendored probe
+/// above and the hosted yarn classic rewriter, so both modes warn alike.
+pub(crate) fn manifest_pins_yarn_classic(manifest: Option<&str>) -> bool {
+    let Some(pm) = manifest
+        .and_then(|pkg| serde_json::from_str::<serde_json::Value>(pkg).ok())
+        .and_then(|v| {
+            v.get("packageManager")
+                .and_then(|p| p.as_str().map(String::from))
+        })
+    else {
+        return false;
+    };
+    let major = pm.trim().strip_prefix("yarn@").map(|rest| {
+        rest.chars()
+            .take_while(char::is_ascii_digit)
+            .collect::<String>()
+    });
+    major.as_deref() == Some("1")
+}
+
 /// Vendoring acquires immutable artifacts from the patch service.
 /// `auto` remains a command-line compatibility alias for `service`.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]