Skip to content

Commit 1dff586

Browse files
committed
Narrow the vendor member refusal
Review follow-ups for the workspace-member refusal: - Only refuse in an npm project (a directory with package.json), so a Python or other service under a pnpm workspace keeps its exit 0. - vendor --check now asks after reading the manifest, so a manifest it cannot read reports that error instead of the member refusal. - vendor --revert is not refused when the member has its own manifest. - Name the pnpm root by its canonical path, like the other roots. - The message no longer claims a member has no lock, since one with a stray lock its manager ignores is refused too. Refs #1921 Assisted-by: Claude Code:claude-opus-5-5
1 parent dcc7c66 commit 1dff586

3 files changed

Lines changed: 71 additions & 24 deletions

File tree

‎crates/socket-patch-cli/src/commands/vendor.rs‎

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1152,7 +1152,7 @@ pub async fn run(args: VendorArgs) -> i32 {
11521152

11531153
// A workspace member's ledger lives at the root: reverting the
11541154
// member's empty one would report success and unwire nothing (#1921).
1155-
if args.revert {
1155+
if args.revert && matches!(manifest_exists(&manifest_path).await, Ok(false)) {
11561156
if let Some(message) = member_refusal(&args.common.cwd, "vendor --revert").await {
11571157
return emit_eject_refusal(&args.common, MEMBER_REFUSAL_CODE, &message);
11581158
}
@@ -1290,20 +1290,20 @@ async fn run_check(args: &VendorArgs) -> i32 {
12901290
if state.entries.is_empty() && jvm_orphan.is_some() {
12911291
return emit_eject_refusal(&args.common, "vendor_ledger_missing", JVM_ORPHAN_DETAIL);
12921292
}
1293-
// A workspace member verifies nothing of its own: the root's ledger
1294-
// and artifacts are the ones the install uses, so a pass here would
1295-
// hide a tampered root (#1921).
1296-
if state.entries.is_empty() && jvm_orphan.is_none() {
1297-
let manifest_path = args.common.resolved_manifest_path();
1298-
if !matches!(manifest_exists(&manifest_path).await, Ok(true)) {
1299-
if let Some(message) = member_refusal(root, "vendor --check").await {
1300-
return emit_eject_refusal(&args.common, MEMBER_REFUSAL_CODE, &message);
1301-
}
1302-
}
1303-
}
13041293
let manifest_path = args.common.resolved_manifest_path();
13051294
let manifest = match read_manifest(&manifest_path).await {
1306-
Ok(m) => m.unwrap_or_default(),
1295+
Ok(Some(m)) => m,
1296+
// A workspace member verifies nothing of its own: the root's
1297+
// ledger and artifacts are the ones the install uses, so a pass
1298+
// here would hide a tampered root (#1921).
1299+
Ok(None) => {
1300+
if state.entries.is_empty() && jvm_orphan.is_none() {
1301+
if let Some(message) = member_refusal(root, "vendor --check").await {
1302+
return emit_eject_refusal(&args.common, MEMBER_REFUSAL_CODE, &message);
1303+
}
1304+
}
1305+
Default::default()
1306+
}
13071307
Err(e) => {
13081308
let err = crate::json_envelope::manifest_load_error(&manifest_path, &e);
13091309
return emit_eject_refusal(&args.common, &err.code, &err.message);
@@ -1498,8 +1498,6 @@ fn artifacts_gitignore_checked(mut ecosystems: impl Iterator<Item = Ecosystem>)
14981498
})
14991499
}
15001500

1501-
/// A refused eject: the JSON error envelope (`status: error`) or an
1502-
/// `Error:` line (printed even under `--silent`). Exit 1; nothing touched.
15031501
/// The refusal code of a vendor run from a workspace member: the same one
15041502
/// vendored `scan` / `get` use for a member with no lock of its own.
15051503
const MEMBER_REFUSAL_CODE: &str = "vendor_lockfile_missing";
@@ -1509,8 +1507,13 @@ const MEMBER_REFUSAL_CODE: &str = "vendor_lockfile_missing";
15091507
/// Callers ask only when `dir` has no hosted pins and no manifest; this
15101508
/// also requires an empty (or absent) vendor ledger, so the run would
15111509
/// otherwise exit 0 having looked at nothing (#1921). An unreadable
1512-
/// ledger is left to the caller's own refusal.
1510+
/// ledger is left to the caller's own refusal. Only an npm project (a
1511+
/// directory holding `package.json`) is a member: another ecosystem's
1512+
/// project nested under a pnpm workspace is its own project.
15131513
async fn member_refusal(dir: &Path, form: &str) -> Option<String> {
1514+
if !dir.join("package.json").is_file() {
1515+
return None;
1516+
}
15141517
if !load_state(dir).await.ok()?.entries.is_empty() {
15151518
return None;
15161519
}
@@ -1521,16 +1524,18 @@ async fn member_refusal(dir: &Path, form: &str) -> Option<String> {
15211524

15221525
fn member_refusal_message(dir: &Path, root: &Path, form: &str) -> String {
15231526
format!(
1524-
"{} is a workspace member with no lockfile, manifest or vendor ledger of its \
1525-
own: the workspace root {} installs it and holds its patches, which `{form}` \
1526-
run here cannot see; run socket-patch from {} (the workspace root); nothing \
1527-
was written",
1527+
"{} is a workspace member with no manifest or vendor ledger of its own: \
1528+
the workspace root {} installs it from its lock and holds its patches, which \
1529+
`{form}` run here cannot see; run socket-patch from {} (the workspace root); \
1530+
nothing was written",
15281531
dir.display(),
15291532
root.display(),
15301533
root.display()
15311534
)
15321535
}
15331536

1537+
/// A refused eject: the JSON error envelope (`status: error`) or an
1538+
/// `Error:` line (printed even under `--silent`). Exit 1; nothing touched.
15341539
fn emit_eject_refusal(common: &GlobalArgs, code: &str, message: &str) -> i32 {
15351540
if common.json {
15361541
let mut env = Envelope::new(Command::Vendor);

‎crates/socket-patch-cli/tests/vendor/vendor_failure_paths.rs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1426,3 +1426,43 @@ fn vendor_outside_a_workspace_keeps_the_no_manifest_exit() {
14261426
let (code, env) = vendor_cli(fx.root(), &["--check"]);
14271427
assert_eq!(code, 0, "{env:#}");
14281428
}
1429+
1430+
/// A non-npm project nested in a pnpm workspace (a Python service beside
1431+
/// the JS packages) is its own project, not a member: its `vendor` forms
1432+
/// keep their exit 0.
1433+
#[test]
1434+
fn vendor_in_a_non_npm_dir_under_a_pnpm_workspace_is_not_refused() {
1435+
let fx = npm_fixture_with_purls(&[]);
1436+
std::fs::remove_file(fx.manifest_path()).unwrap();
1437+
std::fs::write(
1438+
fx.root().join("pnpm-workspace.yaml"),
1439+
"packages:\n - 'packages/*'\n",
1440+
)
1441+
.unwrap();
1442+
std::fs::write(fx.root().join("pnpm-lock.yaml"), "lockfileVersion: '9.0'\n").unwrap();
1443+
let service = fx.root().join("services/api");
1444+
std::fs::create_dir_all(&service).unwrap();
1445+
std::fs::write(
1446+
service.join("pyproject.toml"),
1447+
"[project]\nname = \"api\"\nversion = \"0.1.0\"\n",
1448+
)
1449+
.unwrap();
1450+
1451+
for extra in [&[][..], &["--check"][..], &["--revert"][..]] {
1452+
let (code, env) = vendor_cli(&service, extra);
1453+
assert_eq!(code, 0, "{extra:?}: {env:#}");
1454+
}
1455+
}
1456+
1457+
/// `--revert` in a member that keeps its own manifest (apply mode) is the
1458+
/// documented clean no-op, not the member refusal.
1459+
#[test]
1460+
fn vendor_revert_in_a_member_with_its_own_manifest_is_not_refused() {
1461+
let fx = npm_fixture_with_purls(&[]);
1462+
let member = make_workspace_member(&fx);
1463+
std::fs::create_dir_all(member.join(".socket")).unwrap();
1464+
std::fs::write(member.join(".socket/manifest.json"), b"{\"patches\":{}}\n").unwrap();
1465+
1466+
let (code, env) = vendor_cli(&member, &["--revert"]);
1467+
assert_eq!(code, 0, "{env:#}");
1468+
}

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,19 +136,21 @@ pub async fn npm_family_governing_root(root: &Path) -> Option<(PathBuf, Refusal)
136136
};
137137
if let Some(lock) = pnpm_lock_elsewhere(root).await {
138138
let dir = lock.parent().unwrap_or(&lock);
139+
// Canonical, like the other branches' roots: a relative
140+
// `lockfile-dir` (`../..`) would otherwise name a `..` path.
141+
let pnpm_dir = tokio::fs::canonicalize(dir)
142+
.await
143+
.unwrap_or_else(|_| dir.to_path_buf());
139144
// A `package.json` workspace root nested inside the pnpm lock's
140145
// directory is nearer the member and owns its lock (Bugbot on
141146
// #901); otherwise pnpm's workspace or `lockfile-dir` governs.
142147
if let Some((ws_root, refusal)) = workspace {
143-
let pnpm_dir = tokio::fs::canonicalize(dir)
144-
.await
145-
.unwrap_or_else(|_| dir.to_path_buf());
146148
if ws_root != pnpm_dir && ws_root.starts_with(&pnpm_dir) {
147149
return Some((ws_root, refusal));
148150
}
149151
}
150152
return Some((
151-
dir.to_path_buf(),
153+
pnpm_dir,
152154
Refusal {
153155
code: PNPM_LOCKFILE_ELSEWHERE.to_string(),
154156
message: format!(

0 commit comments

Comments
 (0)