Skip to content

Commit db1d050

Browse files
Fix pnpm lock readers skipping main doc (#1949, #1952) (#1960)
* Start fix for #1949, #1952 Assisted-by: Claude Code:claude-opus-5-5 * Read pnpm locks' project document in every reader pnpm 11+ writes its env lockfile (configDependencies / packageManagerDependencies) as a first YAML document. PnpmLock::parse walked the whole text, so the scan inventory listed config dependencies the install never links while InstalledPackages did not (#1949). It now keeps only entries that start in grammar::main_document, with offsets still relative to the full text, and reads vendored uuids and the packages: sniff from that document. "Does the root lock list a member importer?" was answered by two drifted copies: the formats one read a commented-out ` # sub:` key as a member, the utils one did not, and neither isolated the main document itself (#1952). One predicate in formats::pnpm::workspace now skips comment lines (a column-0 comment no longer ends importers:), and lock_text_has_member_importers applies main_document; the utils copy and the engine's inline wrapper route through it. Fixes #1949 Fixes #1952 Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ff94955 commit db1d050

4 files changed

Lines changed: 125 additions & 52 deletions

File tree

‎crates/socket-patch-core/src/formats/pnpm/mod.rs‎

Lines changed: 62 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -369,8 +369,13 @@ pub(crate) fn check_v9_lock_version(text: &str) -> Result<(), String> {
369369
/// One pnpm lock, parsed once (see the module docs).
370370
pub struct PnpmLock<'t> {
371371
text: &'t str,
372+
/// The project document ([`grammar::main_document`]): pnpm 11+ writes
373+
/// its env lockfile (`configDependencies`, `packageManagerDependencies`)
374+
/// as a first document whose packages the install never links (#1949).
375+
doc: &'t str,
376+
/// Entries of the project document only, with offsets into `text`.
372377
packages: Vec<PnpmPackage<'t>>,
373-
/// [`vendored_npm_uuids`] of the text.
378+
/// [`vendored_npm_uuids`] of the project document.
374379
vendored: HashSet<String>,
375380
}
376381

@@ -386,10 +391,18 @@ pub(crate) struct PnpmTarballRef<'p> {
386391
impl<'t> PnpmLock<'t> {
387392
/// Parse a lock text (any content: a non-pnpm text has no entries).
388393
pub fn parse(text: &'t str) -> Self {
394+
let doc = grammar::main_document(strip_bom(text));
395+
// `doc` is a suffix of `text`: parse the whole text so offsets stay
396+
// relative to it, and keep the entries that start in `doc`.
397+
let doc_start = text.len() - doc.len();
389398
PnpmLock {
390399
text,
391-
packages: pnpm_packages(text),
392-
vendored: vendored_npm_uuids(text),
400+
doc,
401+
packages: pnpm_packages(text)
402+
.into_iter()
403+
.filter(|p| p.entry.offset >= doc_start)
404+
.collect(),
405+
vendored: vendored_npm_uuids(doc),
393406
}
394407
}
395408

@@ -408,7 +421,7 @@ impl<'t> PnpmLock<'t> {
408421
}
409422

410423
fn has_packages_section(&self) -> bool {
411-
self.text
424+
self.doc
412425
.lines()
413426
.any(|l| l.trim_end_matches('\r') == "packages:")
414427
}
@@ -754,6 +767,51 @@ mod tests {
754767
assert!(!PnpmLock::parse(importers).resolves("left-pad", "1.3.0"));
755768
}
756769

770+
/// #1949: a pnpm 11+ lock's env document (config dependencies the
771+
/// install never links) is not part of the inventory: `entries()`,
772+
/// `resolves()`, the vendored uuids and `InstalledPackages` all read
773+
/// the project document alone, and agree with each other.
774+
#[test]
775+
fn env_document_is_not_in_the_inventory() {
776+
let env = format!(
777+
"---\nlockfileVersion: '9.0'\n\nimporters:\n\n .:\n configDependencies:\n cfg-dep:\n specifier: 2.0.0\n version: 2.0.0\n\npackages:\n\n cfg-dep@2.0.0:\n resolution: {{integrity: sha512-c}}\n\n b@file:.socket/vendor/npm/{UUID}/b-1.0.0.tgz:\n resolution: {{integrity: sha512-x, tarball: file:.socket/vendor/npm/{UUID}/b-1.0.0.tgz}}\n version: 1.0.0\n\n"
778+
);
779+
let project = "---\nlockfileVersion: '9.0'\n\nimporters:\n\n .:\n dependencies:\n left-pad:\n specifier: 1.3.0\n version: 1.3.0\n\npackages:\n\n left-pad@1.3.0:\n resolution: {integrity: sha512-x}\n";
780+
for text in [
781+
format!("{env}{project}"),
782+
format!("\u{feff}{env}{project}"),
783+
format!("{env}{project}").replace('\n', "\r\n"),
784+
] {
785+
let lock = PnpmLock::parse(&text);
786+
let names: Vec<_> = lock
787+
.entries()
788+
.unwrap()
789+
.into_iter()
790+
.map(|e| (e.name, e.version))
791+
.collect();
792+
assert_eq!(
793+
names,
794+
[("left-pad".to_string(), "1.3.0".to_string())],
795+
"{text:?}"
796+
);
797+
assert!(lock.resolves("left-pad", "1.3.0"));
798+
assert!(!lock.resolves("cfg-dep", "2.0.0"));
799+
assert!(!lock.vendored_in_use(UUID));
800+
assert!(lock.packages().iter().all(|p| p.entry.offset >= env.len()));
801+
let installed = InstalledPackages::from_lock_text(&text).unwrap();
802+
assert!(installed.contains("left-pad", "1.3.0"));
803+
assert!(!installed.contains("cfg-dep", "2.0.0"));
804+
}
805+
// Offsets still index the whole text.
806+
let text = format!("{env}{project}");
807+
let lock = PnpmLock::parse(&text);
808+
let entry = &lock.packages()[0].entry;
809+
assert_eq!(
810+
&text[entry.offset..entry.offset + entry.body.len()],
811+
entry.body
812+
);
813+
}
814+
757815
#[test]
758816
fn vendored_in_use_reads_v9_and_legacy_keys_and_crlf() {
759817
let v9 = format!(

‎crates/socket-patch-core/src/formats/pnpm/workspace.rs‎

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,14 +151,20 @@ pub(crate) fn is_keyless(text: &str) -> bool {
151151
/// nested package on pnpm 8–10.4 (`packages` defaults to `['.', '**']`) but
152152
/// as the root alone on pnpm >= 10.5, so a lock made from one with member
153153
/// importers comes from pnpm <= 10.4, and the root-only
154-
/// [`PACKAGES_SCAFFOLD`] would drop those members.
155-
pub(crate) fn lock_has_member_importers(lines: &[String]) -> bool {
154+
/// [`PACKAGES_SCAFFOLD`] would drop those members. The same answer marks a
155+
/// root lock as a SHARED workspace lock (`utils::pnpm_workspace`). Comment
156+
/// lines are neither keys nor section ends (#1952).
157+
pub(crate) fn lock_has_member_importers<S: AsRef<str>>(lines: &[S]) -> bool {
156158
// CRLF-blind: a Windows checkout's lock reads like its LF twin.
157159
let mut in_importers = false;
158160
for line in lines {
161+
let line = line.as_ref();
159162
let line = line.strip_suffix('\r').unwrap_or(line);
160-
if !line.is_empty() && !line.starts_with(' ') {
161-
in_importers = line == "importers:";
163+
if line.trim_start().starts_with('#') {
164+
continue;
165+
}
166+
if !line.is_empty() && !line.starts_with([' ', '\t']) {
167+
in_importers = line.trim_end() == "importers:";
162168
} else if in_importers
163169
&& super::lines::parse_key_line(line, 2).is_some_and(|(key, _, _)| key != ".")
164170
{
@@ -168,6 +174,14 @@ pub(crate) fn lock_has_member_importers(lines: &[String]) -> bool {
168174
false
169175
}
170176

177+
/// [`lock_has_member_importers`] of a whole `pnpm-lock.yaml` text: its
178+
/// project document alone ([`super::grammar::main_document`]), so importers
179+
/// of a pnpm 11+ env document never count.
180+
pub(crate) fn lock_text_has_member_importers(text: &str) -> bool {
181+
let doc = super::grammar::main_document(strip_bom(text));
182+
lock_has_member_importers(&doc.split('\n').collect::<Vec<_>>())
183+
}
184+
171185
/// `text` with the contiguous run of lines `block` removed, when that
172186
/// leaves a [`is_keyless`] document: the inverse of a splice that gave a
173187
/// keyless file its first keys. `None` when the run is absent or the rest
@@ -506,6 +520,36 @@ mod tests {
506520
assert!(lock_has_member_importers(&crlf));
507521
}
508522

523+
/// #1952: a commented-out importer is not a member, a column-0 comment
524+
/// does not end `importers:`, and the env document's importers never
525+
/// count; the line and text forms agree.
526+
#[test]
527+
fn member_importers_skip_comments_and_the_env_document() {
528+
let head = "lockfileVersion: '9.0'\n\nimporters:\n\n .:\n dependencies: {}\n";
529+
for (body, members) in [
530+
(" # sub:\n # dependencies: {}\n", false),
531+
("# a note\n sub:\n dependencies: {}\n", true),
532+
(" # nested: comment\n", false),
533+
] {
534+
let text = format!("{head}{body}\npackages: {{}}\n");
535+
assert_eq!(lock_text_has_member_importers(&text), members, "{text:?}");
536+
assert_eq!(
537+
lock_has_member_importers(&lines(&text)),
538+
members,
539+
"{text:?}"
540+
);
541+
}
542+
let env = "---\nlockfileVersion: '9.0'\n\nimporters:\n\n .: {}\n\n tooling:\n configDependencies: {}\n\n";
543+
let project = format!("---\n{head}\npackages: {{}}\n");
544+
assert!(!lock_text_has_member_importers(&format!("{env}{project}")));
545+
assert!(!lock_text_has_member_importers(&format!(
546+
"\u{feff}{env}{project}"
547+
)));
548+
assert!(lock_text_has_member_importers(&format!(
549+
"{env}---\n{head} sub:\n dependencies: {{}}\n"
550+
)));
551+
}
552+
509553
#[test]
510554
fn keyless_documents_and_their_splices() {
511555
for text in [

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2990,13 +2990,8 @@ fn read_advisory_text(view: &ProjectView<'_>, rel: &str) -> Option<String> {
29902990
/// root: with a keyless pnpm-workspace.yaml that marks a pnpm <= 10.4
29912991
/// install (see [`crate::formats::pnpm::workspace::lock_has_member_importers`]).
29922992
fn lock_lists_members(view: &ProjectView<'_>) -> bool {
2993-
read_advisory_text(view, "pnpm-lock.yaml").is_some_and(|text| {
2994-
let doc =
2995-
crate::formats::pnpm::grammar::main_document(crate::formats::text::strip_bom(&text));
2996-
crate::formats::pnpm::workspace::lock_has_member_importers(
2997-
&crate::formats::pnpm::lines::split_lines(doc),
2998-
)
2999-
})
2993+
read_advisory_text(view, "pnpm-lock.yaml")
2994+
.is_some_and(|text| crate::formats::pnpm::workspace::lock_text_has_member_importers(&text))
30002995
}
30012996

30022997
/// The ancestor `pnpm-workspace.yaml` governing a disk project's pnpm

‎crates/socket-patch-core/src/utils/pnpm_workspace.rs‎

Lines changed: 13 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -262,37 +262,6 @@ pub fn shared_lockfile_disabled(workspace: &str, npmrc: Option<&str>) -> bool {
262262
.is_some_and(|setting| setting.as_bool() == Some(false))
263263
}
264264

265-
/// Whether a root lock is a SHARED workspace lock: its `importers:` names a
266-
/// project other than the root (`.`). Such a lock is what pnpm installs
267-
/// every member from, whatever the settings say now, so member locks
268-
/// beside it are stale leftovers.
269-
pub fn root_lock_lists_members(lock: &str) -> bool {
270-
let mut in_importers = false;
271-
for line in lock.lines() {
272-
let line = line.strip_suffix('\r').unwrap_or(line);
273-
if !line.starts_with([' ', '\t']) && !line.trim().is_empty() {
274-
in_importers = line.trim_end() == "importers:";
275-
continue;
276-
}
277-
if !in_importers {
278-
continue;
279-
}
280-
// An importer key sits at exactly two spaces.
281-
let Some(key) = line
282-
.strip_prefix(" ")
283-
.filter(|k| !k.starts_with([' ', '#']))
284-
else {
285-
continue;
286-
};
287-
let key = key.split(':').next().unwrap_or("").trim();
288-
let key = key.trim_matches(['\'', '"']);
289-
if !key.is_empty() && key != "." {
290-
return true;
291-
}
292-
}
293-
false
294-
}
295-
296265
/// Whether pnpm's project finder lists the directory at `rel` (relative
297266
/// to the workspace root, one entry per component) under `globs`
298267
/// (`pnpm-workspace.yaml` `packages:`, as [`read_package_globs`] reads them), as
@@ -473,7 +442,13 @@ async fn workspace_members(view: &ProjectView<'_>) -> Members {
473442
return Members::Shared;
474443
}
475444
let root_lock = view.read_text(PNPM_LOCK).await.ok();
476-
if root_lock.as_deref().is_some_and(root_lock_lists_members) {
445+
// A root lock whose importers name a project other than `.` is a
446+
// SHARED workspace lock: pnpm installs every member from it, whatever
447+
// the settings say now, so member locks beside it are stale leftovers.
448+
if root_lock
449+
.as_deref()
450+
.is_some_and(crate::formats::pnpm::workspace::lock_text_has_member_importers)
451+
{
477452
return Members::Shared;
478453
}
479454
let globs = match read_package_globs(&workspace) {
@@ -1217,18 +1192,19 @@ mod tests {
12171192

12181193
#[test]
12191194
fn a_root_lock_listing_member_importers_is_shared() {
1195+
use crate::formats::pnpm::workspace::lock_text_has_member_importers;
12201196
let root_only = "lockfileVersion: '9.0'\n\nimporters:\n\n .:\n dependencies:\n a:\n specifier: 1.0.0\n version: 1.0.0\n\npackages:\n\n a@1.0.0:\n resolution: {integrity: sha512-x}\n";
1221-
assert!(!root_lock_lists_members(root_only));
1222-
assert!(!root_lock_lists_members(
1197+
assert!(!lock_text_has_member_importers(root_only));
1198+
assert!(!lock_text_has_member_importers(
12231199
"lockfileVersion: '9.0'\n\nimporters:\n\n .: {}\n"
12241200
));
1225-
assert!(!root_lock_lists_members(
1201+
assert!(!lock_text_has_member_importers(
12261202
"lockfileVersion: 5.4\n\ndependencies:\n a: 1.0.0\n"
12271203
));
1228-
assert!(root_lock_lists_members(
1204+
assert!(lock_text_has_member_importers(
12291205
"lockfileVersion: '9.0'\r\n\r\nimporters:\r\n\r\n .: {}\r\n\r\n packages/a:\r\n dependencies: {}\r\n"
12301206
));
1231-
assert!(root_lock_lists_members(
1207+
assert!(lock_text_has_member_importers(
12321208
"lockfileVersion: '6.0'\nimporters:\n '.': {}\n 'packages/a': {}\n"
12331209
));
12341210
}

0 commit comments

Comments
 (0)