From 06b7f60ab03778ee9e45c94fe1bca72189372590 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 16:25:33 +0000 Subject: [PATCH 1/6] Start fix for #385, #474 Assisted-by: Claude Code:claude-opus-5-5 From 7b30eb92f9cf6ffd6a6b20df6069ca2ace69e277 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 16:35:07 +0000 Subject: [PATCH 2/6] Let vendored Python reverts keep sibling edits Rolling back a vendored Hatch project failed with "pyproject.toml changed since patching" after a routine release bump, a comment on `name`, or a new dependency next to the patched one (#385). A vendored PEP 723 script lock stayed vendored for good once `uv add --script` added an unrelated requirement (#474). Both reverts go through one three-way TOML merge. It paired array elements by position, called any length change drift, and checked lock-package name/version on every table, including [project]. The merge now restores only the elements vendoring changed, finding each one in the live array by its text or its identity (PEP 508 name, or table name + version). Siblings the user added, dropped or moved are left alone. A vendored element that was edited, re-resolved, removed or duplicated is still reported as drift. Assisted-by: Claude Code:claude-opus-5-5 --- .../src/vendor/pypi_hatch.rs | 50 ++++ .../socket-patch-core/src/vendor/pypi_lock.rs | 260 ++++++++++++++++-- 2 files changed, 290 insertions(+), 20 deletions(-) diff --git a/crates/socket-patch-core/src/vendor/pypi_hatch.rs b/crates/socket-patch-core/src/vendor/pypi_hatch.rs index aa8528b69..e67050712 100644 --- a/crates/socket-patch-core/src/vendor/pypi_hatch.rs +++ b/crates/socket-patch-core/src/vendor/pypi_hatch.rs @@ -464,4 +464,54 @@ mod tests { ORIGINAL ); } + + /// #385: ordinary pyproject edits after vendoring (a release bump, a + /// comment on `name`, a new sibling dependency) must not block rollback. + #[tokio::test] + async fn revert_keeps_unrelated_project_edits() { + let original = + "[project]\nname = \"app\"\nversion = \"0.1.0\"\ndependencies = [\"six==1.16.0\"]\n"; + let edits: [(&str, &str); 4] = [ + ("version = \"0.1.0\"", "version = \"0.2.0\""), + ("name = \"app\"", "name = \"app\" # renamed soon"), + ("dependencies = [", "dependencies = [\"idna==3.7\", "), + ( + "version = \"0.1.0\"\n", + "version = \"0.3.0\"\ndescription = \"x\"\n", + ), + ]; + for (from, to) in edits { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + tokio::fs::write(root.join("pyproject.toml"), original) + .await + .unwrap(); + let project = load(root, "six", "1.16.0", UUID).await.unwrap(); + let wheel = format!(".socket/vendor/pypi/{UUID}/six-1.16.0-py2.py3-none-any.whl"); + let wiring = wire(&project, root, "six", "1.16.0", &wheel, &"0".repeat(64)) + .await + .unwrap(); + let entry = entry(UUID, "six", &wheel, &"0".repeat(64), wiring); + let mut state = VendorState::default(); + state.entries.insert("six".into(), entry.clone()); + save_state(root, &state).await.unwrap(); + let patched = tokio::fs::read_to_string(root.join("pyproject.toml")) + .await + .unwrap(); + assert!(patched.contains(&wheel)); + assert!(patched.contains(from), "{patched}"); + tokio::fs::write(root.join("pyproject.toml"), patched.replacen(from, to, 1)) + .await + .unwrap(); + let outcome = revert(&entry, root, false).await; + assert!(outcome.success, "{from} -> {to}: {:?}", outcome.error); + assert_eq!( + tokio::fs::read_to_string(root.join("pyproject.toml")) + .await + .unwrap(), + original.replacen(from, to, 1), + "{from} -> {to}" + ); + } + } } diff --git a/crates/socket-patch-core/src/vendor/pypi_lock.rs b/crates/socket-patch-core/src/vendor/pypi_lock.rs index 929d41a3c..ad6744468 100644 --- a/crates/socket-patch-core/src/vendor/pypi_lock.rs +++ b/crates/socket-patch-core/src/vendor/pypi_lock.rs @@ -405,10 +405,81 @@ fn equal_item(left: Option<&Item>, right: Option<&Item>) -> bool { left.map(item_text) == right.map(item_text) } -fn same_identity(live: &dyn TableLike, expected: &dyn TableLike) -> bool { - ["name", "version"] - .iter() - .all(|key| equal_item(live.get(key), expected.get(key))) +/// What makes an array element "the same entry" across the three +/// documents: the canonical name plus `version` for a `name`d table (a lock +/// package, a `[manifest] requirements` element), the PEP 508 project name +/// for a requirement string. `None` means only an exact textual match can +/// pair the element. +fn table_identity(table: &dyn TableLike) -> Option { + let name = table.get("name")?.as_str()?; + let version = table.get("version").and_then(Item::as_str).unwrap_or(""); + Some(format!("{}\0{version}", canonicalize_pypi_name(name))) +} + +fn value_identity(value: &Value) -> Option { + match value { + Value::String(text) => { + let text = text.value().trim_start(); + let end = text + .find(|ch: char| !(ch.is_ascii_alphanumeric() || matches!(ch, '-' | '_' | '.'))) + .unwrap_or(text.len()); + (end > 0).then(|| canonicalize_pypi_name(&text[..end])) + } + Value::InlineTable(table) => table_identity(table), + _ => None, + } +} + +/// `value` without its own surrounding whitespace and comments, so an +/// element reads the same wherever it sits in a (re)flowed array. +fn bare_value(value: &Value) -> String { + let mut value = value.clone(); + value.decor_mut().clear(); + value.to_string() +} + +fn bare_table(table: &Table) -> String { + let mut table = table.clone(); + table.decor_mut().clear(); + item_text(&Item::Table(table)) +} + +/// Pairs every element the vendoring changed (`original[i] != new[i]`) +/// with the one live element that still is that entry: the unique live +/// element textually equal to `new[i]`, else the unique one with its +/// identity. Elements the vendoring left alone are not looked up at all, so +/// the user may add, drop or reorder them freely. `None` (drift) when a +/// changed element is gone, ambiguous, or two of them claim one live slot. +fn pair_changed( + original: &[String], + new: &[String], + new_identity: &[Option], + live: &[String], + live_identity: &[Option], +) -> Option> { + let mut pairs = Vec::new(); + let mut claimed = BTreeSet::new(); + for index in (0..new.len()).filter(|&index| original[index] != new[index]) { + let unique = |found: Vec| (found.len() == 1).then(|| found[0]); + let textual: Vec = (0..live.len()) + .filter(|&slot| live[slot] == new[index]) + .collect(); + let slot = if textual.is_empty() { + let identity = new_identity[index].as_ref()?; + unique( + (0..live.len()) + .filter(|&slot| live_identity[slot].as_ref() == Some(identity)) + .collect(), + )? + } else { + unique(textual)? + }; + if !claimed.insert(slot) { + return None; + } + pairs.push((slot, index)); + } + Some(pairs) } fn restore_table(live: &mut dyn TableLike, original: &dyn TableLike, new: &dyn TableLike) -> bool { @@ -454,6 +525,16 @@ fn restore_table(live: &mut dyn TableLike, original: &dyn TableLike, new: &dyn T drifted } +/// `original` in place of `live`, keeping the live element's own spacing +/// and comments unless they are still the vendored ones. +fn replace_value(live: &mut Value, original: &Value, new: &Value) { + let decor = live.decor().clone(); + *live = original.clone(); + if decor != *new.decor() { + *live.decor_mut() = decor; + } +} + fn restore_value(live: &mut Value, original: &Value, new: &Value) -> bool { if original.to_string() == new.to_string() || live.to_string() == original.to_string() { return false; @@ -467,20 +548,40 @@ fn restore_value(live: &mut Value, original: &Value, new: &Value) -> bool { original.as_inline_table(), new.as_inline_table(), ) { - if !same_identity(live, new) { - return true; - } return restore_table(live, original, new); } if let (Some(live), Some(original), Some(new)) = (live.as_array_mut(), original.as_array(), new.as_array()) { - if live.len() != new.len() || original.len() != new.len() { + if original.len() != new.len() { return true; } + let original: Vec<&Value> = original.iter().collect(); + let new: Vec<&Value> = new.iter().collect(); + let Some(pairs) = pair_changed( + &original + .iter() + .map(|value| bare_value(value)) + .collect::>(), + &new.iter() + .map(|value| bare_value(value)) + .collect::>(), + &new.iter() + .map(|value| value_identity(value)) + .collect::>(), + &live.iter().map(bare_value).collect::>(), + &live.iter().map(value_identity).collect::>(), + ) else { + return true; + }; let mut drifted = false; - for ((live, original), new) in live.iter_mut().zip(original.iter()).zip(new.iter()) { - drifted |= restore_value(live, original, new); + for (slot, index) in pairs { + let current = live.get_mut(slot).expect("paired slot is in range"); + if bare_value(current) == bare_value(new[index]) { + replace_value(current, original[index], new[index]); + } else { + drifted |= restore_value(current, original[index], new[index]); + } } return drifted; } @@ -500,9 +601,6 @@ fn restore_item(live: &mut Item, original: &Item, new: &Item) -> bool { original.as_table_like(), new.as_table_like(), ) { - if !same_identity(live, new) { - return true; - } return restore_table(live, original, new); } if let (Some(live), Some(original), Some(new)) = ( @@ -510,16 +608,34 @@ fn restore_item(live: &mut Item, original: &Item, new: &Item) -> bool { original.as_array_of_tables(), new.as_array_of_tables(), ) { - if live.len() != new.len() || original.len() != new.len() { + if original.len() != new.len() { return true; } + let original: Vec<&Table> = original.iter().collect(); + let new: Vec<&Table> = new.iter().collect(); + let Some(pairs) = pair_changed( + &original + .iter() + .map(|table| bare_table(table)) + .collect::>(), + &new.iter() + .map(|table| bare_table(table)) + .collect::>(), + &new.iter() + .map(|table| table_identity(*table)) + .collect::>(), + &live.iter().map(bare_table).collect::>(), + &live + .iter() + .map(|table| table_identity(table)) + .collect::>(), + ) else { + return true; + }; let mut drifted = false; - for ((live, original), new) in live.iter_mut().zip(original.iter()).zip(new.iter()) { - if same_identity(live, new) { - drifted |= restore_table(live, original, new); - } else { - drifted = true; - } + for (slot, index) in pairs { + let current = live.get_mut(slot).expect("paired slot is in range"); + drifted |= restore_table(current, original[index], new[index]); } return drifted; } @@ -867,6 +983,110 @@ mod tests { assert_eq!(restored, edited); } + /// #474: `uv add --script` after vendoring adds a sibling element to the + /// script lock's `[manifest] requirements` array (and a `[[package]]`). + /// Only the vendored element is restored; the user's additions stay. + #[test] + fn added_sibling_requirement_does_not_block_script_lock_revert() { + let original = "version = 1\nrequires-python = \">=3.9\"\n\n[manifest]\nrequirements = [\n { name = \"python-dateutil\", specifier = \"==2.8.2\" },\n { name = \"six\", specifier = \"==1.16.0\" },\n]\n\n[[package]]\nname = \"python-dateutil\"\nversion = \"2.8.2\"\nsource = { registry = \"https://pypi.org/simple\" }\n\n[[package]]\nname = \"six\"\nversion = \"1.16.0\"\nsource = { registry = \"https://pypi.org/simple\" }\n"; + let new = original + .replace( + "{ name = \"six\", specifier = \"==1.16.0\" }", + "{ name = \"six\", path = \".socket/vendor/pypi/u/six-1.16.0-py2.py3-none-any.whl\" }", + ) + .replace( + "name = \"six\"\nversion = \"1.16.0\"\nsource = { registry = \"https://pypi.org/simple\" }", + "name = \"six\"\nversion = \"1.16.0\"\nsource = { path = \".socket/vendor/pypi/u/six-1.16.0-py2.py3-none-any.whl\" }", + ); + let add_idna = |text: &str| { + text.replace( + " { name = \"python-dateutil\", specifier = \"==2.8.2\" },\n", + " { name = \"idna\", specifier = \"==3.7\" },\n { name = \"python-dateutil\", specifier = \"==2.8.2\" },\n", + ) + .replacen( + "[[package]]\nname = \"python-dateutil\"", + "[[package]]\nname = \"idna\"\nversion = \"3.7\"\nsource = { registry = \"https://pypi.org/simple\" }\n\n[[package]]\nname = \"python-dateutil\"", + 1, + ) + }; + let live = add_idna(&new); + let (restored, drifted) = restore_document(&live, original, &new).unwrap(); + assert!(!drifted, "{restored}"); + assert_eq!(restored, add_idna(original)); + } + + /// Appending to (or prepending to) a recorded array of strings keeps the + /// user's element and restores only the vendored one. + #[test] + fn sibling_strings_added_or_removed_around_the_vendored_element() { + let original = "dependencies = [\"one==1\", \"six==1.16.0\", \"two==2\"]\n"; + let new = "dependencies = [\"one==1\", \"six @ file:///v/six.whl\", \"two==2\"]\n"; + for (live, expected) in [ + ( + "dependencies = [\"idna==3.7\", \"one==1\", \"six @ file:///v/six.whl\", \"two==2\"]\n", + "dependencies = [\"idna==3.7\", \"one==1\", \"six==1.16.0\", \"two==2\"]\n", + ), + ( + "dependencies = [\"six @ file:///v/six.whl\", \"two==2\"]\n", + "dependencies = [\"six==1.16.0\", \"two==2\"]\n", + ), + ( + "dependencies = [\"one==1\", \"six @ file:///v/six.whl\", \"two==2\", \"idna==3.7\"]\n", + "dependencies = [\"one==1\", \"six==1.16.0\", \"two==2\", \"idna==3.7\"]\n", + ), + ] { + let (restored, drifted) = restore_document(live, original, new).unwrap(); + assert!(!drifted, "{live}"); + assert_eq!(restored, expected); + } + } + + /// The vendored element itself changed or vanished: still drift, and the + /// live text is kept. + #[test] + fn vendored_element_edited_or_removed_is_still_drift() { + let original = "dependencies = [\"one==1\", \"six==1.16.0\"]\n"; + let new = "dependencies = [\"one==1\", \"six @ file:///v/six.whl\"]\n"; + for live in [ + "dependencies = [\"one==1\", \"idna==3.7\"]\n", + "dependencies = [\"one==1\", \"six==1.17.0\", \"idna==3.7\"]\n", + "dependencies = [\"six @ file:///v/six.whl\", \"six @ file:///v/six.whl\"]\n", + ] { + let (restored, drifted) = restore_document(live, original, new).unwrap(); + assert!(drifted, "{live}"); + assert_eq!(restored, live); + } + } + + /// A re-resolved package (new version) is not the vendored entry, even + /// when siblings were added too. + #[test] + fn re_resolved_package_with_added_sibling_is_drift() { + let patched = rewrite_python_lock( + LOCK, + "one", + "1", + ArtifactSource::Path(".socket/vendor/one-1-py3-none-any.whl"), + "first", + ) + .unwrap() + .unwrap(); + let edited = format!( + "{}\n[[packages]]\nname = \"three\"\nversion = \"3\"\n", + patched.replace("version = \"1\"", "version = \"1.1\"") + ); + let (restored, drifted) = restore_document(&edited, LOCK, &patched).unwrap(); + assert!(drifted); + assert_eq!(restored, edited); + let added = format!("{patched}\n[[packages]]\nname = \"three\"\nversion = \"3\"\n"); + let (restored, drifted) = restore_document(&added, LOCK, &patched).unwrap(); + assert!(!drifted); + assert_eq!( + restored, + format!("{LOCK}\n[[packages]]\nname = \"three\"\nversion = \"3\"\n") + ); + } + #[test] fn recorded_files_cannot_escape_the_project() { for file in [ From bb4372e68bdc60875b275fce0d7cc24a4fc1baef Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 17:05:21 +0000 Subject: [PATCH 3/6] Test real uv/Hatch reverts after sibling edits Adds end-to-end steps with the real tools: the vendored uv script lane now runs `uv add --script tool.py idna==3.7` on a copy before `vendor --revert` (#474), and the vendored Hatch project lane bumps the release and adds a dependency before reverting (#385). Both check the user's edits survive and the vendored wiring is gone. Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_vex_build/hatch.rs | 46 ++++++++++ .../tests/vex_e2e_common/uv.rs | 84 +++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs b/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs index a7877c9ed..55b9177cd 100644 --- a/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs +++ b/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs @@ -323,6 +323,52 @@ fn flow(flavor: Flavor, mode: Mode) { "pass", ); + // ── #385: everyday pyproject edits must not block the revert ────── + // A release bump and a new sibling dependency after vendoring: the + // revert restores six's pin and drops the permission, keeping both. + if mode == Mode::Vendored && flavor == Flavor::Project { + let edited = tmp.path().join("edited"); + copy_tree(&project, &edited, &[]); + let edit = |text: &str| { + text.replacen("version = \"0.1.0\"", "version = \"0.2.0\"", 1) + .replacen("dependencies = [", "dependencies = [\"idna==3.7\", ", 1) + }; + std::fs::write(edited.join("pyproject.toml"), edit(&wired)).unwrap(); + let out = std::process::Command::new(vex_e2e_common::binary()) + .args(["vendor", "--revert", "--json", "--cwd"]) + .arg(&edited) + .current_dir(&edited) + .output() + .unwrap(); + assert_eq!( + out.status.code(), + Some(0), + "{what}: revert after project edits: {}", + out_text(&out) + ); + let native = &flavor.native()[0].1; + assert_eq!( + std::fs::read_to_string(edited.join("pyproject.toml")).unwrap(), + edit(native), + "{what}: revert after project edits: {}", + out_text(&out) + ); + assert!( + !edited + .join(".socket/vendor/pypi") + .join(mode.uuid()) + .exists(), + "{what}: the vendored artifact was kept" + ); + record( + "hatch", + &version, + &format!("{cell}/{}", mode.label()), + "revert-after-project-edits", + "pass", + ); + } + // ── fresh checkout, real `hatch env create` ─────────────────────── let fresh = tmp.path().join("fresh"); copy_tree(&project, &fresh, &[".socket/manifest.json"]); diff --git a/crates/socket-patch-cli/tests/vex_e2e_common/uv.rs b/crates/socket-patch-cli/tests/vex_e2e_common/uv.rs index 1bc5ac69d..52d8360f1 100644 --- a/crates/socket-patch-cli/tests/vex_e2e_common/uv.rs +++ b/crates/socket-patch-cli/tests/vex_e2e_common/uv.rs @@ -1560,6 +1560,11 @@ pub fn run_lane(suite: &str, uv: &Uv, mode: Mode, lane: Lane) { &|step, result| report.row(step, result), ); + // ── 5b. a sibling dependency added after vendoring (#474) ───────── + if mode == Mode::Vendored && lane == Lane::Script { + script_sibling_revert(uv, &report, &proj, tmp.path()); + } + // ── 6. the real revert ──────────────────────────────────────────── // Vendored: `vendor --revert` restores every wiring file byte for // byte. Hosted (v5): `rollback` rewrites each pin back to the DEFAULT @@ -1682,6 +1687,85 @@ pub fn run_lane(suite: &str, uv: &Uv, mode: Mode, lane: Lane) { report.row("revert", "byte-identical"); } +/// #474: `uv add --script` after vendoring adds an unrelated requirement +/// to the script and its lock (a new `[manifest] requirements` element and +/// `[[package]]`). `vendor --revert` must still restore six's registry +/// wiring and keep the user's addition, leaving a lock uv accepts as is. +/// Runs on a copy so the byte-identical revert below is unaffected. +fn script_sibling_revert(uv: &Uv, report: &Report<'_>, proj: &Path, tmp: &Path) { + let dir = tmp.join("sibling"); + std::fs::create_dir_all(&dir).unwrap(); + for f in [SCRIPT, "tool.py.lock"] { + std::fs::copy(proj.join(f), dir.join(f)).unwrap(); + } + copy_tree(&proj.join(".socket"), &dir.join(".socket")); + let cache = tmp.join("sibling-cache"); + let out = uv.run_py(&dir, &["add", "--script", SCRIPT, "idna==3.7"], &cache); + if !ok(&out) { + report.row("sibling-revert", "n/a (`uv add --script` failed)"); + println!("{}", dump(&out)); + return; + } + let lock = std::fs::read_to_string(dir.join("tool.py.lock")).unwrap(); + assert!( + lock.contains("name = \"idna\"") && lock.contains(".socket/vendor"), + "{}: uv add did not keep the vendored lock:\n{lock}", + report.what("sibling-revert") + ); + let out = socket_patch( + &dir, + &[ + "vendor", + "--revert", + "--json", + "--cwd", + dir.to_str().unwrap(), + ], + ); + let text = format!("{}\n{}", String::from_utf8_lossy(&out.stdout), dump(&out)); + assert_eq!( + out.status.code(), + Some(0), + "{}:\n{text}", + report.what("sibling-revert") + ); + assert!( + !text.contains("vendor_lock_entry_drifted"), + "{}: an added sibling counted as drift:\n{text}", + report.what("sibling-revert") + ); + for f in [SCRIPT, "tool.py.lock"] { + let body = std::fs::read_to_string(dir.join(f)).unwrap(); + assert!( + !body.contains(".socket/vendor") && body.contains("idna"), + "{}: {f} still vendored or lost idna:\n{body}", + report.what("sibling-revert") + ); + } + assert!( + !dir.join(".socket/vendor/pypi") + .join(Mode::Vendored.uuid()) + .exists(), + "{}: the vendored artifact was kept", + report.what("sibling-revert") + ); + let reverted = std::fs::read(dir.join("tool.py.lock")).unwrap(); + let out = uv.run_py(&dir, &["lock", "--script", SCRIPT], &cache); + assert!( + ok(&out), + "{}: uv lock --script:\n{}", + report.what("sibling-revert"), + dump(&out) + ); + assert_eq!( + String::from_utf8_lossy(&std::fs::read(dir.join("tool.py.lock")).unwrap()), + String::from_utf8_lossy(&reverted), + "{}: uv rewrote the reverted lock", + report.what("sibling-revert") + ); + report.row("sibling-revert", "restored, user addition kept"); +} + // ── production legs ──────────────────────────────────────────────────── /// The uv program a production leg drives: `SOCKET_PATCH_UV_E2E_BIN`, else From 1e6967d91dcb3fc76bfb2ba0717b6b0a5ea6300a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 17:21:29 +0000 Subject: [PATCH 4/6] Keep drift when a renamed pin hides behind a twin Review found a gap in the new array merge: if the user renamed the vendored element past recognition and also added a sibling equal to the original pin, the revert paired the sibling as "already restored", reported success, and deleted the vendored artifact the renamed line still pointed at. An "already restored" pairing now only counts when every other live element is accounted for. Otherwise the revert reports drift and keeps the artifact. Elements already back to their original text are also skipped regardless of their spacing. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-core/src/vendor/pypi_lock.rs | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/socket-patch-core/src/vendor/pypi_lock.rs b/crates/socket-patch-core/src/vendor/pypi_lock.rs index ad6744468..686319339 100644 --- a/crates/socket-patch-core/src/vendor/pypi_lock.rs +++ b/crates/socket-patch-core/src/vendor/pypi_lock.rs @@ -450,6 +450,12 @@ fn bare_table(table: &Table) -> String { /// identity. Elements the vendoring left alone are not looked up at all, so /// the user may add, drop or reorder them freely. `None` (drift) when a /// changed element is gone, ambiguous, or two of them claim one live slot. +/// +/// An identity match that already reads as the original proves nothing on +/// its own: the vendored element may have been edited beyond recognition +/// (renamed) while the user added a sibling equal to the original. So that +/// "already restored" pairing only stands when no other live element is +/// unaccounted for, i.e. neither paired nor one of the recorded elements. fn pair_changed( original: &[String], new: &[String], @@ -459,6 +465,7 @@ fn pair_changed( ) -> Option> { let mut pairs = Vec::new(); let mut claimed = BTreeSet::new(); + let mut already_original = false; for index in (0..new.len()).filter(|&index| original[index] != new[index]) { let unique = |found: Vec| (found.len() == 1).then(|| found[0]); let textual: Vec = (0..live.len()) @@ -466,11 +473,13 @@ fn pair_changed( .collect(); let slot = if textual.is_empty() { let identity = new_identity[index].as_ref()?; - unique( + let slot = unique( (0..live.len()) .filter(|&slot| live_identity[slot].as_ref() == Some(identity)) .collect(), - )? + )?; + already_original |= live[slot] == original[index]; + slot } else { unique(textual)? }; @@ -479,7 +488,10 @@ fn pair_changed( } pairs.push((slot, index)); } - Some(pairs) + let unaccounted = (0..live.len()).any(|slot| { + !claimed.contains(&slot) && !original.contains(&live[slot]) && !new.contains(&live[slot]) + }); + (!(already_original && unaccounted)).then_some(pairs) } fn restore_table(live: &mut dyn TableLike, original: &dyn TableLike, new: &dyn TableLike) -> bool { @@ -577,6 +589,9 @@ fn restore_value(live: &mut Value, original: &Value, new: &Value) -> bool { let mut drifted = false; for (slot, index) in pairs { let current = live.get_mut(slot).expect("paired slot is in range"); + if bare_value(current) == bare_value(original[index]) { + continue; + } if bare_value(current) == bare_value(new[index]) { replace_value(current, original[index], new[index]); } else { @@ -635,6 +650,9 @@ fn restore_item(live: &mut Item, original: &Item, new: &Item) -> bool { let mut drifted = false; for (slot, index) in pairs { let current = live.get_mut(slot).expect("paired slot is in range"); + if bare_table(current) == bare_table(original[index]) { + continue; + } drifted |= restore_table(current, original[index], new[index]); } return drifted; @@ -1051,6 +1069,8 @@ mod tests { "dependencies = [\"one==1\", \"idna==3.7\"]\n", "dependencies = [\"one==1\", \"six==1.17.0\", \"idna==3.7\"]\n", "dependencies = [\"six @ file:///v/six.whl\", \"six @ file:///v/six.whl\"]\n", + // Renamed past recognition, with a sibling equal to the original. + "dependencies = [\"one==1\", \"other @ file:///v/six.whl\", \"six==1.16.0\"]\n", ] { let (restored, drifted) = restore_document(live, original, new).unwrap(); assert!(drifted, "{live}"); @@ -1058,6 +1078,26 @@ mod tests { } } + /// A pin the user already put back by hand still reverts cleanly (the + /// rest of the wiring is restored), but not when another, unknown + /// element could be the vendored one renamed (Bugbot on #481). + #[test] + fn already_restored_element_is_trusted_only_without_unknown_siblings() { + let original = "dependencies = [\"one==1\", \"six==1.16.0\"]\n"; + let new = "dependencies = [\"one==1\", \"six @ file:///v/six.whl\"]\n"; + let (restored, drifted) = restore_document(original, original, new).unwrap(); + assert!(!drifted); + assert_eq!(restored, original); + let live = "dependencies = [\"six==1.16.0\", \"one==1\"]\n"; + let (restored, drifted) = restore_document(live, original, new).unwrap(); + assert!(!drifted); + assert_eq!(restored, live); + let live = "dependencies = [\"one==1\", \"six==1.16.0\", \"idna==3.7\"]\n"; + let (restored, drifted) = restore_document(live, original, new).unwrap(); + assert!(drifted); + assert_eq!(restored, live); + } + /// A re-resolved package (new version) is not the vendored entry, even /// when siblings were added too. #[test] From d1698d36bf69a5ab13d93732f783281d2bca72bf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 14:35:31 +0000 Subject: [PATCH 5/6] Refuse reverts that leave a vendored wheel in use Review found that a user-added copy of the vendored requirement (for example with a `python_version` marker) survives the array merge as a new sibling. The revert then reported success and deleted the wheel that line still installs from. Before a Hatch or Python-lock revert counts as complete, it now checks that the restored file no longer mentions this patch's `.socket/vendor/pypi/` directory. If it does, Hatch fails and the lock revert reports `vendor_lock_entry_drifted`, so the file and the artifact are kept. Adds unit tests and a real-Hatch e2e step that fail without the check. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01CPpPuqqqgLkPXkcpsFfKFo --- .../tests/e2e_vex_build/hatch.rs | 46 +++++++++++++ .../src/vendor/pypi_hatch.rs | 65 ++++++++++++++++++- .../socket-patch-core/src/vendor/pypi_lock.rs | 33 ++++++++++ 3 files changed, 143 insertions(+), 1 deletion(-) diff --git a/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs b/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs index 4552aac3d..042001150 100644 --- a/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs +++ b/crates/socket-patch-cli/tests/e2e_vex_build/hatch.rs @@ -367,6 +367,52 @@ fn flow(flavor: Flavor, mode: Mode) { "revert-after-project-edits", "pass", ); + + // Review on #481: an added, marked copy of the vendored requirement + // still installs from the wheel, so the revert must refuse and keep + // both the file and the artifact. + let copied = tmp.path().join("copied"); + copy_tree(&project, &copied, &[]); + let start = wired.find("\"six @").unwrap() + 1; + let requirement = &wired[start..start + wired[start..].find('"').unwrap()]; + let with_copy = wired.replacen( + "dependencies = [", + &format!("dependencies = [\"{requirement} ; python_version >= '3.8'\", "), + 1, + ); + std::fs::write(copied.join("pyproject.toml"), &with_copy).unwrap(); + let out = std::process::Command::new(vex_e2e_common::binary()) + .args(["vendor", "--revert", "--json", "--cwd"]) + .arg(&copied) + .current_dir(&copied) + .output() + .unwrap(); + assert_ne!( + out.status.code(), + Some(0), + "{what}: revert with a copied vendored requirement: {}", + out_text(&out) + ); + assert_eq!( + std::fs::read_to_string(copied.join("pyproject.toml")).unwrap(), + with_copy, + "{what}: revert with a copied vendored requirement" + ); + assert!( + copied + .join(".socket/vendor/pypi") + .join(mode.uuid()) + .exists(), + "{what}: the still-referenced artifact was deleted: {}", + out_text(&out) + ); + record( + "hatch", + &version, + &format!("{cell}/{}", mode.label()), + "revert-refuses-copied-reference", + "pass", + ); } // ── fresh checkout, real `hatch env create` ─────────────────────── diff --git a/crates/socket-patch-core/src/vendor/pypi_hatch.rs b/crates/socket-patch-core/src/vendor/pypi_hatch.rs index e67050712..a14464697 100644 --- a/crates/socket-patch-core/src/vendor/pypi_hatch.rs +++ b/crates/socket-patch-core/src/vendor/pypi_hatch.rs @@ -251,9 +251,21 @@ pub(super) async fn revert(entry: &VendorEntry, root: &Path, dry_run: bool) -> R return RevertOutcome::failed("missing Hatch wiring document"); }; match super::pypi_lock::restore_document(live, original, new) { - Ok((restored, false)) => { + Ok((restored, false)) + if !super::pypi_lock::still_references_artifact( + &restored, + original, + &entry.uuid, + ) => + { edits.insert(record.file.clone(), restored); } + Ok((_, false)) => { + return RevertOutcome::failed(format!( + "{} still references the vendored artifact after restoring the recorded entries", + record.file + )) + } Ok((_, true)) => { return RevertOutcome::failed(format!("{} changed since patching", record.file)) } @@ -514,4 +526,55 @@ mod tests { ); } } + + /// Review on #481: a user-added copy of the vendored requirement (here + /// with an environment marker) survives the merge as a sibling, so the + /// revert must refuse rather than delete the wheel it installs from. + #[tokio::test] + async fn revert_refuses_while_an_added_line_references_the_artifact() { + let original = + "[project]\nname = \"app\"\nversion = \"0.1.0\"\ndependencies = [\"six==1.16.0\"]\n"; + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + tokio::fs::write(root.join("pyproject.toml"), original) + .await + .unwrap(); + let project = load(root, "six", "1.16.0", UUID).await.unwrap(); + let wheel = format!(".socket/vendor/pypi/{UUID}/six-1.16.0-py2.py3-none-any.whl"); + let wiring = wire(&project, root, "six", "1.16.0", &wheel, &"0".repeat(64)) + .await + .unwrap(); + let entry = entry(UUID, "six", &wheel, &"0".repeat(64), wiring); + let patched = tokio::fs::read_to_string(root.join("pyproject.toml")) + .await + .unwrap(); + let start = patched.find("\"six @").unwrap(); + let end = start + 1 + patched[start + 1..].find('"').unwrap(); + let requirement = &patched[start + 1..end]; + let edited = patched.replacen( + "dependencies = [", + &format!("dependencies = [\"{requirement} ; python_version >= '3.8'\", "), + 1, + ); + tokio::fs::write(root.join("pyproject.toml"), &edited) + .await + .unwrap(); + let outcome = revert(&entry, root, false).await; + assert!(!outcome.success, "{:?}", outcome.error); + assert!( + outcome + .error + .as_deref() + .unwrap_or_default() + .contains("still references the vendored artifact"), + "{:?}", + outcome.error + ); + assert_eq!( + tokio::fs::read_to_string(root.join("pyproject.toml")) + .await + .unwrap(), + edited + ); + } } diff --git a/crates/socket-patch-core/src/vendor/pypi_lock.rs b/crates/socket-patch-core/src/vendor/pypi_lock.rs index 686319339..b360436bf 100644 --- a/crates/socket-patch-core/src/vendor/pypi_lock.rs +++ b/crates/socket-patch-core/src/vendor/pypi_lock.rs @@ -700,6 +700,16 @@ fn allowed_file(file: &str, kind: &str) -> bool { || (kind == SCRIPT_KIND && file.ends_with(".py"))) } +/// Whether `restored` still points at this patch's vendored artifact +/// directory although `original` did not. The merge keeps elements it did +/// not record (a user's added sibling), and one of those may be a copy of +/// the vendored requirement: reporting the revert complete would delete a +/// wheel that line still installs from. +pub(super) fn still_references_artifact(restored: &str, original: &str, uuid: &str) -> bool { + let needle = format!("vendor/pypi/{uuid}"); + restored.contains(&needle) && !original.contains(&needle) +} + pub(super) async fn revert_python_locks( entry: &VendorEntry, root: &Path, @@ -759,6 +769,14 @@ pub(super) async fn revert_python_locks( record.file ), )); + } else if still_references_artifact(&restored, original, &entry.uuid) { + warnings.push(VendorWarning::new( + "vendor_lock_entry_drifted", + format!( + "{} still references the vendored artifact after restoring the recorded entries", + record.file + ), + )); } if restored != live { edits.push((record.file.clone(), live, restored)); @@ -1033,6 +1051,21 @@ mod tests { assert_eq!(restored, add_idna(original)); } + /// Review on #481: an added copy of the vendored element survives the + /// merge, so the revert must see the leftover artifact reference. + #[test] + fn leftover_artifact_reference_is_detected() { + let original = "dependencies = [\"six==1.16.0\"]\n"; + let new = "dependencies = [\"six @ file:///p/.socket/vendor/pypi/u/six.whl\"]\n"; + let live = "dependencies = [\"six @ file:///p/.socket/vendor/pypi/u/six.whl ; python_version >= '3.8'\", \"six @ file:///p/.socket/vendor/pypi/u/six.whl\"]\n"; + let (restored, drifted) = restore_document(live, original, new).unwrap(); + assert!(!drifted, "{restored}"); + assert!(still_references_artifact(&restored, original, "u")); + assert!(!still_references_artifact(original, original, "u")); + // Another patch's artifact (a stacked vendoring) is not this one's. + assert!(!still_references_artifact(&restored, original, "v")); + } + /// Appending to (or prepending to) a recorded array of strings keeps the /// user's element and restores only the vendored one. #[test] From f3f20c61540872cb5d3768b3e52797619b0022cc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 14:53:35 +0000 Subject: [PATCH 6/6] Don't restore a removed pin into a namesake row Bugbot found that when the user removes the vendored array row, the merge could still pair it by name with another row (a requirement's identity ignores `specifier`). It then filled the original fields into that row and reported a clean revert, rewriting the user's entry. A table paired only by identity must now still hold at least one field exactly as the vendoring wrote it, such as the vendored `path` or `source`. Otherwise the revert reports drift and keeps the file and the artifact. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01CPpPuqqqgLkPXkcpsFfKFo --- .../socket-patch-core/src/vendor/pypi_lock.rs | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/crates/socket-patch-core/src/vendor/pypi_lock.rs b/crates/socket-patch-core/src/vendor/pypi_lock.rs index b360436bf..3d298432d 100644 --- a/crates/socket-patch-core/src/vendor/pypi_lock.rs +++ b/crates/socket-patch-core/src/vendor/pypi_lock.rs @@ -537,6 +537,21 @@ fn restore_table(live: &mut dyn TableLike, original: &dyn TableLike, new: &dyn T drifted } +/// Whether `live` still holds at least one field exactly as the vendoring +/// wrote it. A table paired only by its name (a requirement's identity +/// ignores `specifier`) may be a namesake the user added after removing +/// the vendored row: filling the original fields into that one would +/// rewrite the user's entry and report a clean revert. +fn keeps_a_vendored_field( + live: &dyn TableLike, + original: &dyn TableLike, + new: &dyn TableLike, +) -> bool { + new.iter().any(|(key, after)| { + !equal_item(original.get(key), Some(after)) && equal_item(live.get(key), Some(after)) + }) +} + /// `original` in place of `live`, keeping the live element's own spacing /// and comments unless they are still the vendored ones. fn replace_value(live: &mut Value, original: &Value, new: &Value) { @@ -594,6 +609,16 @@ fn restore_value(live: &mut Value, original: &Value, new: &Value) -> bool { } if bare_value(current) == bare_value(new[index]) { replace_value(current, original[index], new[index]); + } else if let (Some(live), Some(before), Some(after)) = ( + current.as_inline_table(), + original[index].as_inline_table(), + new[index].as_inline_table(), + ) { + if !keeps_a_vendored_field(live, before, after) { + drifted = true; + continue; + } + drifted |= restore_value(current, original[index], new[index]); } else { drifted |= restore_value(current, original[index], new[index]); } @@ -653,6 +678,10 @@ fn restore_item(live: &mut Item, original: &Item, new: &Item) -> bool { if bare_table(current) == bare_table(original[index]) { continue; } + if !keeps_a_vendored_field(current, original[index], new[index]) { + drifted = true; + continue; + } drifted |= restore_table(current, original[index], new[index]); } return drifted; @@ -1051,6 +1080,41 @@ mod tests { assert_eq!(restored, add_idna(original)); } + /// Bugbot on #481: with the vendored row removed, a namesake the user + /// added (identity ignores `specifier`) must not be filled in with the + /// original fields and reported as a clean revert. + #[test] + fn removed_vendored_table_does_not_restore_into_a_namesake() { + let original = + "requirements = [{ name = \"one\" }, { name = \"six\", specifier = \"==1.16.0\" }]\n"; + let new = "requirements = [{ name = \"one\" }, { name = \"six\", path = \".socket/vendor/pypi/u/six.whl\" }]\n"; + for live in [ + "requirements = [{ name = \"one\" }, { name = \"six\" }]\n", + "requirements = [{ name = \"one\" }, { name = \"six\", marker = \"python_version >= '3.8'\" }]\n", + ] { + let (restored, drifted) = restore_document(live, original, new).unwrap(); + assert!(drifted, "{live}"); + assert_eq!(restored, live); + } + let tables = |six: &str| { + format!("[[package]]\nname = \"one\"\n\n[[package]]\nname = \"six\"\n{six}") + }; + let original = tables("specifier = \"==1.16.0\"\n"); + let new = tables("path = \".socket/vendor/pypi/u/six.whl\"\n"); + let live = tables(""); + let (restored, drifted) = restore_document(&live, &original, &new).unwrap(); + assert!(drifted); + assert_eq!(restored, live); + // The vendored row itself, edited only elsewhere, still reverts. + let live = tables("path = \".socket/vendor/pypi/u/six.whl\"\nmarker = \"x\"\n"); + let (restored, drifted) = restore_document(&live, &original, &new).unwrap(); + assert!(!drifted, "{restored}"); + assert_eq!( + restored, + tables("marker = \"x\"\nspecifier = \"==1.16.0\"\n") + ); + } + /// Review on #481: an added copy of the vendored element survives the /// merge, so the revert must see the leftover artifact reference. #[test]