From 706e0782736f5c903cae63b7ac018be7d67f68b0 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 14 Aug 2026 06:52:54 -0700 Subject: [PATCH] fix(hosted): honest warnings for pnpm/yarn/bun and CRLF locks Warning hygiene only - no rewrite behavior changes. - `redirect_npm_no_lockfile` fired on every pnpm/yarn/bun/Rush hosted run because the npm rewriter warned whenever package-lock.json was absent, ignoring the sibling lock that was successfully rewritten. It now fires only when NO npm-family lockfile exists at all, so a clean pnpm/yarn/bun/Rush run no longer trains users to ignore the warnings channel where the real signals live. - A CRLF yarn berry lock was refused as "cacheKey is `(missing)`" even though its cacheKey IS 10c0 - the `\n\n` block grammar never splits a CRLF file - sending Windows users chasing yarn cache config. The refusal now names the line endings (redirect_yarn_berry_crlf_unsupported); same fail-closed outcome. - A CRLF pnpm-lock.yaml surfaced per-dep as a misleading `redirect_pnpm_entry_not_found` ("no inline resolution") though the entry exists; it now gets a dedicated redirect_pnpm_crlf_unsupported warning naming the file, and still redirects nothing (fail-closed). New tests pin the EXACT warning set of clean success runs (npm: empty; pnpm: exactly the trust-lockfile caveat) at both the rewriter unit level and the --json envelope level, plus the two CRLF diagnoses and the sibling-lock gating (yarn-only, Rush-nested, and lockfile-less legs). The exact-set envelope test fails on the pre-fix code. Co-authored-by: Claude Fable 5 --- .../tests/in_process_redirect.rs | 48 +++ .../src/patch/redirect/mod.rs | 273 +++++++++++++++++- 2 files changed, 312 insertions(+), 9 deletions(-) diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index d302aa48..6ae7189d 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -1649,6 +1649,54 @@ async fn pnpm_lock_redirect_warns_to_trust_lockfile() { ); } +/// A clean, fully-successful hosted run must carry an EXACT warning set — not +/// merely "contains X". Presence-only assertions let spurious warnings ride +/// along unnoticed: every pnpm/yarn/bun/Rush run shipped a bogus +/// `redirect_npm_no_lockfile` ("no package-lock.json present") because the +/// npm rewriter warned whenever ITS lock was absent, with no regard for the +/// sibling lock that was successfully rewritten. npm success = zero warnings; +/// pnpm success = exactly the trust-lockfile install caveat. +#[tokio::test] +#[serial] +async fn clean_success_warning_set_is_exact_for_npm_and_pnpm() { + let server = MockServer::start().await; + mock_discovery(&server).await; + mock_reference(&server).await; + mock_view(&server).await; + + // npm project (package-lock.json): NO warnings of any kind. + let npm = tempfile::tempdir().unwrap(); + write_project(npm.path()); + let env = run_redirect_subprocess(npm.path(), &server.uri()); + assert_eq!(env["status"], "success", "envelope: {env}"); + assert_eq!( + env["redirect"]["redirected"], 1, + "anchor: the npm lock must have been redirected: {env}" + ); + assert_eq!( + warning_codes(&env), + Vec::::new(), + "a clean npm success must emit an EMPTY warning set: {env}" + ); + + // pnpm project (pnpm-lock.yaml only — no package-lock.json, by design): + // EXACTLY the pnpm >=11 trust-lockfile caveat, nothing else. + let pnpm = tempfile::tempdir().unwrap(); + write_pnpm_project(pnpm.path()); + let env = run_redirect_subprocess(pnpm.path(), &server.uri()); + assert_eq!(env["status"], "success", "envelope: {env}"); + assert_eq!( + env["redirect"]["redirected"], 1, + "anchor: the pnpm lock must have been redirected: {env}" + ); + assert_eq!( + warning_codes(&env), + vec!["redirect_pnpm_trust_lockfile".to_string()], + "a clean pnpm success must emit EXACTLY the trust-lockfile caveat \ + (regression: spurious redirect_npm_no_lockfile): {env}" + ); +} + /// Cargo's hosted redirect wires the managed sparse registry into /// `.cargo/config.toml` — but a project carrying the LEGACY extensionless /// `.cargo/config` is one cargo READS INSTEAD (it warns about the duplicate diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 0ff40eb5..d33ed0a4 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -175,10 +175,25 @@ fn rewrite_npm_lock( .into_iter() .find(|f| files.contains_key(*f)); let Some(lockfile) = lockfile else { - result.warnings.push(RewriteWarning { - code: "redirect_npm_no_lockfile".into(), - detail: "no package-lock.json / npm-shrinkwrap.json present".into(), + // Another npm-family lock (pnpm — root or nested Rush —, yarn, bun) + // owns the redirect for these deps and its rewriter emits its own + // per-dep diagnostics; warning "no package-lock.json" on every + // successful pnpm/yarn/bun/Rush run is pure noise that trains users + // to ignore the warnings channel. Only warn when NO npm-family + // lockfile exists at all. + let sibling_lock_present = files.keys().any(|k| { + k == "yarn.lock" + || k == "bun.lock" + || k == "bun.lockb" + || k == "pnpm-lock.yaml" + || k.ends_with("/pnpm-lock.yaml") }); + if !sibling_lock_present { + result.warnings.push(RewriteWarning { + code: "redirect_npm_no_lockfile".into(), + detail: "no package-lock.json / npm-shrinkwrap.json present".into(), + }); + } return; }; let Ok(mut lock) = serde_json::from_str::(&files[lockfile]) else { @@ -672,11 +687,30 @@ fn rewrite_pnpm_lock( return; } // Work on an editable copy of each lock so a single dep can be rewritten - // in whichever locks contain it. - let mut contents: Vec<(&String, String, bool)> = lock_keys - .iter() - .map(|k| (*k, files[*k].clone(), false)) - .collect(); + // in whichever locks contain it. A CRLF lock can never match the rewrite + // grammar (its pattern anchors on `):\n`, but every CRLF line puts a `\r` + // byte before the `\n`), so the miss used to surface per-dep as a + // misleading `redirect_pnpm_entry_not_found`. Name the real cause instead + // and skip the lock (fail-closed, as before). + let mut contents: Vec<(&String, String, bool)> = Vec::new(); + for k in &lock_keys { + let content = files[*k].clone(); + if content.contains("\r\n") { + result.warnings.push(RewriteWarning { + code: "redirect_pnpm_crlf_unsupported".into(), + detail: format!( + "{k} has CRLF (Windows) line endings; the redirect's \ + byte-surgical rewrite only supports LF — normalize the \ + file to LF line endings and re-run" + ), + }); + continue; + } + contents.push((*k, content, false)); + } + if contents.is_empty() { + return; + } for dep in &npm { let fname = full_name(dep); let Some(sha512) = dep.integrity.sha512.clone() else { @@ -910,7 +944,23 @@ fn rewrite_yarn_berry( return; } - // Whole-file gates: refuse any lock whose cache checksum we can't reproduce + // Whole-file gates. A CRLF lock collapses the `\n\n` block grammar (a + // `\r\n\r\n` file contains no `\n\n`), so `berry_cache_key` used to come + // back None and the refusal below misdiagnosed a perfectly good + // `cacheKey: 10c0` lock as "cacheKey is `(missing)`" — sending Windows + // users chasing yarn cache config instead of line endings. Same + // fail-closed outcome, honest diagnosis. + if content.contains("\r\n") { + result.warnings.push(RewriteWarning { + code: "redirect_yarn_berry_crlf_unsupported".into(), + detail: "yarn.lock has CRLF (Windows) line endings; the redirect's \ + byte-surgical rewrite only supports LF — normalize the \ + file to LF line endings and re-run" + .into(), + }); + return; + } + // Refuse any lock whose cache checksum we can't reproduce // offline. A guessed `checksum:` bricks installs (YN0018). let key = berry_cache_key(content); if key.as_deref() != Some(YARN_BERRY_SUPPORTED_CACHE_KEY) { @@ -3682,4 +3732,209 @@ snapshots: second.edits ); } + + fn pnpm_v9_lock(name: &str, version: &str) -> String { + format!( + "lockfileVersion: '9.0' + +importers: + .: + dependencies: + {name}: + specifier: {version} + version: {version} + +packages: + {name}@{version}: + resolution: {{integrity: sha512-UPSTREAM==}} + +snapshots: + {name}@{version}: {{}} +" + ) + } + + /// A clean, fully-successful redirect must emit EXACTLY zero rewrite + /// warnings — for the npm lock AND for a pnpm-only project. The pnpm leg + /// regressed silently for a long time: `rewrite_npm_lock` pushed a + /// spurious `redirect_npm_no_lockfile` onto every pnpm/yarn/bun/Rush run + /// because those projects (correctly) have no package-lock.json. + #[test] + fn clean_success_run_emits_no_warnings_for_npm_and_pnpm() { + let ovr = npm_override( + "left-pad", + "1.3.0", + "http://patch.test/left-pad-1.3.0.tgz", + "sha512-PATCHED==", + ); + + // npm: package-lock.json only. + let mut files = BTreeMap::new(); + files.insert( + "package-lock.json".to_string(), + r#"{ + "name": "app", + "lockfileVersion": 3, + "packages": { + "": { "name": "app", "version": "0.0.0" }, + "node_modules/left-pad": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz", + "integrity": "sha512-UPSTREAM==" + } + } +} +"# + .to_string(), + ); + let r = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr)); + assert!( + r.files.contains_key("package-lock.json"), + "anchor: the npm lock must have been rewritten" + ); + assert_eq!( + warning_codes(&r), + Vec::<&str>::new(), + "a clean npm success must emit NO warnings" + ); + + // pnpm: pnpm-lock.yaml only — no package-lock.json exists, by design. + let mut files = BTreeMap::new(); + files.insert( + "pnpm-lock.yaml".to_string(), + pnpm_v9_lock("left-pad", "1.3.0"), + ); + let r = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr)); + assert!( + r.files.contains_key("pnpm-lock.yaml"), + "anchor: the pnpm lock must have been rewritten" + ); + assert_eq!( + warning_codes(&r), + Vec::<&str>::new(), + "a clean pnpm success must emit NO warnings (regression: spurious \ + redirect_npm_no_lockfile)" + ); + } + + /// The `redirect_npm_no_lockfile` warning is gated on NO npm-family lock + /// being present at all: yarn-only and Rush nested-pnpm-only projects are + /// handled by their own rewriters and must not carry npm noise, while a + /// genuinely lockfile-less project still gets the warning. + #[test] + fn npm_no_lockfile_warning_gated_on_sibling_npm_family_locks() { + let ovr = npm_override( + "left-pad", + "1.3.0", + "http://patch.test/left-pad-1.3.0.tgz", + "sha512-PATCHED==", + ); + + // yarn classic only: rewritten by the yarn rewriter, zero warnings. + let mut files = BTreeMap::new(); + files.insert( + "yarn.lock".to_string(), + "left-pad@^1.3.0:\n version \"1.3.0\"\n resolved \ + \"https://registry.yarnpkg.com/left-pad/-/left-pad-1.3.0.tgz#ab\"\n \ + integrity sha512-UPSTREAM==\n" + .to_string(), + ); + let r = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr)); + assert!( + r.files.contains_key("yarn.lock"), + "anchor: yarn.lock must have been rewritten" + ); + assert_eq!( + warning_codes(&r), + Vec::<&str>::new(), + "a clean yarn-classic success must emit NO warnings" + ); + + // Rush: only a NESTED pnpm lock (no root lock of any kind). + let mut files = BTreeMap::new(); + files.insert( + "common/config/rush/pnpm-lock.yaml".to_string(), + pnpm_v9_lock("left-pad", "1.3.0"), + ); + let r = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr)); + assert!( + r.files.contains_key("common/config/rush/pnpm-lock.yaml"), + "anchor: the nested Rush lock must have been rewritten" + ); + assert_eq!( + warning_codes(&r), + Vec::<&str>::new(), + "a clean Rush success must emit NO warnings" + ); + + // No lockfile anywhere: the warning still fires (unchanged contract). + let files = BTreeMap::new(); + let r = rewrite_registry_redirect(&files, std::slice::from_ref(&ovr)); + assert!( + warning_codes(&r).contains(&"redirect_npm_no_lockfile"), + "a lockfile-less project must still warn: {:?}", + r.warnings + ); + } + + /// A CRLF berry lock must be diagnosed as a line-ending problem, not as + /// `cacheKey is \`(missing)\`` — the lock's cacheKey IS 10c0; only the + /// `\n\n` block grammar fails on `\r\n\r\n`. Fail-closed either way. + #[test] + fn berry_crlf_lock_diagnosed_as_crlf_not_missing_cache_key() { + let checksum = format!("10c0/{}", "7".repeat(128)); + let ovr = berry_override("left-pad", "1.3.0", "http://p.test/lp.tgz", &checksum); + let mut files = BTreeMap::new(); + files.insert( + "yarn.lock".to_string(), + berry_lock("10c0").replace('\n', "\r\n"), + ); + let mut r = RewriteResult::default(); + rewrite_yarn_berry(&files, std::slice::from_ref(&ovr), &mut r); + assert!(r.files.is_empty(), "CRLF lock must not be rewritten"); + assert_eq!( + warning_codes(&r), + vec!["redirect_yarn_berry_crlf_unsupported"], + "the refusal must name CRLF, not the cache key: {:?}", + r.warnings + ); + assert!( + r.warnings[0].detail.contains("CRLF"), + "detail must name the line endings: {}", + r.warnings[0].detail + ); + } + + /// A CRLF pnpm lock gets a dedicated CRLF warning instead of the + /// misleading per-dep `redirect_pnpm_entry_not_found` (the entry exists; + /// only the LF-anchored grammar cannot see it). Fail-closed either way. + #[test] + fn pnpm_crlf_lock_gets_dedicated_crlf_warning() { + let ovr = npm_override( + "left-pad", + "1.3.0", + "http://patch.test/left-pad-1.3.0.tgz", + "sha512-PATCHED==", + ); + let mut files = BTreeMap::new(); + files.insert( + "pnpm-lock.yaml".to_string(), + pnpm_v9_lock("left-pad", "1.3.0").replace('\n', "\r\n"), + ); + let mut r = RewriteResult::default(); + rewrite_pnpm_lock(&files, std::slice::from_ref(&ovr), &mut r); + assert!(r.files.is_empty(), "CRLF lock must not be rewritten"); + assert_eq!( + warning_codes(&r), + vec!["redirect_pnpm_crlf_unsupported"], + "the refusal must name CRLF, not entry-not-found: {:?}", + r.warnings + ); + assert!( + r.warnings[0].detail.contains("pnpm-lock.yaml") + && r.warnings[0].detail.contains("CRLF"), + "detail must name the file and the line endings: {}", + r.warnings[0].detail + ); + } }