From 3cf1c41b534007040ce51540a65a907450e113f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 08:23:54 +0000 Subject: [PATCH 1/7] Start fix for #819 Assisted-by: Claude Code:claude-opus-5-5 From df7d35c371a2f66636ef37a7c92c720a05f7b757 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 08:32:38 +0000 Subject: [PATCH 2/7] Add failing tests for stale Python bytecode A same-size patch applied in the same second pip compiled the module leaves the timestamp .pyc valid, so Python keeps running the unpatched code. Rollback has the mirror problem. These tests cover the core write path and a real CPython venv end to end (#819). Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_pypi_bytecode.rs | 213 ++++++++++++++++++ crates/socket-patch-core/src/patch/apply.rs | 155 +++++++++++++ .../socket-patch-core/src/patch/rollback.rs | 52 +++++ 3 files changed, 420 insertions(+) create mode 100644 crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs diff --git a/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs b/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs new file mode 100644 index 000000000..bee8d6fc3 --- /dev/null +++ b/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs @@ -0,0 +1,213 @@ +//! Agent-mode PyPI apply and rollback against real CPython bytecode (#819). +//! +//! pip byte-compiles every installed module into `__pycache__/*.pyc`. +//! Without `SOURCE_DATE_EPOCH` those files are timestamp-validated: CPython +//! reuses the bytecode while the source's mtime (whole seconds) and size +//! match the `.pyc` header. A same-size patch written in the same second as +//! the install therefore kept the unpatched bytecode running while `apply` +//! reported success, and a rollback in the same second as the apply kept +//! the patched bytecode running. +//! +//! The race is made deterministic by putting the source's mtime back on +//! the second the bytecode was compiled against after each write: exactly +//! what an apply or rollback inside that second leaves on disk. +//! +//! A real venv and interpreter, a hand-installed `dupkit 1.0.0`, a staged +//! manifest + blobs and `--offline`. Skipped when no Python 3 is on PATH. + +mod common; + +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::time::{Duration, SystemTime}; + +use serde_json::json; + +const PURL: &str = "pkg:pypi/dupkit@1.0.0"; +const UUID: &str = "81981981-9819-4819-8819-819819819819"; +const FILE: &str = "dupkit/__init__.py"; +// Same length, so a timestamp `.pyc` cannot tell them apart by size. +const ORIGINAL: &[u8] = b"VERSION = 'original'\n"; +const PATCHED: &[u8] = b"VERSION = 'patchedX'\n"; + +/// Env vars that change where or whether CPython reads and writes +/// bytecode, or which interpreter / site dirs are used. +const PYTHON_VARS: &[&str] = &[ + "PYTHONDONTWRITEBYTECODE", + "PYTHONPYCACHEPREFIX", + "SOURCE_DATE_EPOCH", + "PYTHONHOME", + "PYTHONPATH", + "PYTHONUSERBASE", + "VIRTUAL_ENV", + "CONDA_PREFIX", + "WORKON_HOME", + "PIPENV_ACTIVE", +]; + +fn python_on_path() -> Option<&'static str> { + ["python3", "python"].into_iter().find(|py| { + Command::new(py) + .args(["-c", "import sys; assert sys.version_info >= (3, 7)"]) + .output() + .is_ok_and(|o| o.status.success()) + }) +} + +fn python(py: &Path, args: &[&str]) -> String { + let mut cmd = Command::new(py); + cmd.args(args); + for name in PYTHON_VARS { + cmd.env_remove(name); + } + let out = cmd.output().expect("run python"); + assert!( + out.status.success(), + "python {args:?} failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).trim().to_string() +} + +fn socket_patch(cwd: &Path, args: &[&str]) { + let mut cmd = common::hermetic::command(&common::binary()); + cmd.args(args).current_dir(cwd); + for name in PYTHON_VARS { + cmd.env_remove(name); + } + let out = cmd.output().expect("run socket-patch"); + assert_eq!( + out.status.code(), + Some(0), + "socket-patch {args:?} failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); +} + +fn set_mtime(path: &Path, mtime: SystemTime) { + std::fs::File::options() + .write(true) + .open(path) + .unwrap() + .set_modified(mtime) + .unwrap(); +} + +/// Whole-second mtime: the precision a timestamp `.pyc` records. +fn second_of(path: &Path) -> SystemTime { + let secs = std::fs::metadata(path) + .unwrap() + .modified() + .unwrap() + .duration_since(SystemTime::UNIX_EPOCH) + .unwrap() + .as_secs(); + SystemTime::UNIX_EPOCH + Duration::from_secs(secs) +} + +/// What `import dupkit` runs, through the venv's interpreter. +fn imported_version(py: &Path) -> String { + python(py, &["-c", "import dupkit; print(dupkit.VERSION)"]) +} + +fn stage_patch(project: &Path) { + let socket = project.join(".socket"); + std::fs::create_dir_all(&socket).unwrap(); + let manifest = json!({ + "patches": { + PURL: { + "uuid": UUID, + "exportedAt": "2026-01-01T00:00:00Z", + "files": { FILE: { + "beforeHash": common::git_sha256(ORIGINAL), + "afterHash": common::git_sha256(PATCHED), + }}, + "vulnerabilities": {}, + "description": "synthetic same-size bytecode test patch", + "license": "MIT", + "tier": "free", + } + } + }); + std::fs::write( + socket.join("manifest.json"), + serde_json::to_vec_pretty(&manifest).unwrap(), + ) + .unwrap(); + common::write_blob(&socket, &common::git_sha256(ORIGINAL), ORIGINAL); + common::write_blob(&socket, &common::git_sha256(PATCHED), PATCHED); +} + +/// #819: a same-size apply (and rollback) in the same second as the +/// bytecode was compiled must still change what `import` runs. +#[test] +fn same_second_same_size_apply_and_rollback_change_imported_code() { + let Some(system_py) = python_on_path() else { + eprintln!("SKIP: no python3 on PATH"); + return; + }; + let tmp = tempfile::tempdir().unwrap(); + let project = tmp.path().join("proj"); + std::fs::create_dir_all(&project).unwrap(); + std::fs::write(project.join("requirements.txt"), "dupkit==1.0.0\n").unwrap(); + let venv = project.join(".venv"); + python( + Path::new(system_py), + &["-m", "venv", "--without-pip", venv.to_str().unwrap()], + ); + let py: PathBuf = if cfg!(windows) { + venv.join("Scripts").join("python.exe") + } else { + venv.join("bin").join("python") + }; + let site = PathBuf::from(python( + &py, + &["-c", "import sysconfig; print(sysconfig.get_paths()['purelib'])"], + )); + + // Install dupkit 1.0.0 the way pip leaves it: dist-info, module, and + // timestamp bytecode compiled from the module. + let dist = site.join("dupkit-1.0.0.dist-info"); + std::fs::create_dir_all(&dist).unwrap(); + std::fs::write( + dist.join("METADATA"), + "Metadata-Version: 2.1\nName: dupkit\nVersion: 1.0.0\n", + ) + .unwrap(); + let module = site.join(FILE); + std::fs::create_dir_all(module.parent().unwrap()).unwrap(); + std::fs::write(&module, ORIGINAL).unwrap(); + python( + &py, + &["-m", "compileall", "-q", module.parent().unwrap().to_str().unwrap()], + ); + assert_eq!(imported_version(&py), "original"); + stage_patch(&project); + + // Apply in the same second the install compiled against. + let installed_second = second_of(&module); + socket_patch(&project, &["apply", "--offline"]); + assert_eq!(std::fs::read(&module).unwrap(), PATCHED); + set_mtime(&module, installed_second); + assert_eq!( + imported_version(&py), + "patchedX", + "apply reported the module patched, but Python still runs the \ + install's bytecode" + ); + + // The import above compiled bytecode for the patched source. Roll back + // in that same second. + let patched_second = second_of(&module); + socket_patch(&project, &["rollback", "--offline", "--yes"]); + assert_eq!(std::fs::read(&module).unwrap(), ORIGINAL); + set_mtime(&module, patched_second); + assert_eq!( + imported_version(&py), + "original", + "rollback restored the source, but Python still runs the patched \ + bytecode" + ); +} diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index bfb668c09..2bd800e3e 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -3510,4 +3510,159 @@ mod tests { assert!(result.success, "{:?}", result.error); assert_eq!(std::fs::read(real.join("index.js")).unwrap(), patched); } + + /// Write `name` (and its parents) under `root` with placeholder bytes. + async fn touch(root: &Path, names: &[&str]) { + for name in names { + let path = root.join(name); + tokio::fs::create_dir_all(path.parent().unwrap()) + .await + .unwrap(); + tokio::fs::write(&path, b"pyc").await.unwrap(); + } + } + + /// #819: rewriting a `.py` file removes the bytecode CPython compiled + /// from the old source. A timestamp `.pyc` only records the source's + /// mtime (whole seconds) and size, so a same-size patch in the same + /// second as the install would otherwise keep the unpatched bytecode + /// running. Bytecode for other modules stays. + #[tokio::test] + async fn test_apply_file_patch_py_removes_stale_bytecode() { + let site = tempfile::tempdir().unwrap(); + let original = b"AUTHOR = 'original'\n"; + let patched = b"AUTHOR = 'patchedX'\n"; + assert_eq!(original.len(), patched.len()); + touch( + site.path(), + &[ + "pkg/__pycache__/mod.cpython-311.pyc", + "pkg/__pycache__/mod.cpython-311.opt-1.pyc", + "pkg/__pycache__/mod.cpython-313.opt-2.pyc", + "pkg/__pycache__/modx.cpython-311.pyc", + "pkg/__pycache__/other.cpython-311.pyc", + "pkg/mod.pyc", + "pkg/mod.pyo", + "pkg/other.pyc", + ], + ) + .await; + tokio::fs::write(site.path().join("pkg/mod.py"), original) + .await + .unwrap(); + + apply_file_patch_at( + site.path(), + "pkg/mod.py", + patched, + &compute_git_sha256_from_bytes(patched), + ) + .await + .unwrap(); + + let pkg = site.path().join("pkg"); + assert_eq!(std::fs::read(pkg.join("mod.py")).unwrap(), patched); + for stale in [ + "__pycache__/mod.cpython-311.pyc", + "__pycache__/mod.cpython-311.opt-1.pyc", + "__pycache__/mod.cpython-313.opt-2.pyc", + "mod.pyc", + "mod.pyo", + ] { + assert!(!pkg.join(stale).exists(), "{stale} must be removed"); + } + for kept in [ + "__pycache__/modx.cpython-311.pyc", + "__pycache__/other.cpython-311.pyc", + "other.pyc", + ] { + assert!(pkg.join(kept).exists(), "{kept} must be kept"); + } + } + + /// #819: a top-level module (`six.py` directly in site-packages) has + /// its bytecode in site-packages' own `__pycache__`. + #[tokio::test] + async fn test_apply_file_patch_top_level_py_removes_stale_bytecode() { + let site = tempfile::tempdir().unwrap(); + let patched = b"x = 2\n"; + touch(site.path(), &["__pycache__/six.cpython-312.pyc"]).await; + tokio::fs::write(site.path().join("six.py"), b"x = 1\n") + .await + .unwrap(); + + apply_file_patch_at( + site.path(), + "six.py", + patched, + &compute_git_sha256_from_bytes(patched), + ) + .await + .unwrap(); + + assert!(!site.path().join("__pycache__/six.cpython-312.pyc").exists()); + } + + /// Only a `.py` write touches bytecode: patching `mod.js` (or + /// `mod.pyi`) leaves `mod.*.pyc` alone. + #[tokio::test] + async fn test_apply_file_patch_non_py_keeps_bytecode() { + let site = tempfile::tempdir().unwrap(); + let patched = b"patched"; + touch( + site.path(), + &["pkg/__pycache__/mod.cpython-311.pyc", "pkg/mod.pyc"], + ) + .await; + for name in ["pkg/mod.js", "pkg/mod.pyi"] { + tokio::fs::write(site.path().join(name), b"original") + .await + .unwrap(); + apply_file_patch_at( + site.path(), + name, + patched, + &compute_git_sha256_from_bytes(patched), + ) + .await + .unwrap(); + } + + assert!(site + .path() + .join("pkg/__pycache__/mod.cpython-311.pyc") + .exists()); + assert!(site.path().join("pkg/mod.pyc").exists()); + } + + /// A read-only `__pycache__` (a root-owned system site-packages, or a + /// Nix/Go-style read-only tree) is relaxed for the delete and put back + /// exactly as found. + #[cfg(unix)] + #[tokio::test] + async fn test_apply_file_patch_py_removes_bytecode_in_readonly_cache() { + use std::os::unix::fs::PermissionsExt; + let site = tempfile::tempdir().unwrap(); + let patched = b"x = 2\n"; + let cache = site.path().join("__pycache__"); + touch(site.path(), &["__pycache__/m.cpython-312.pyc"]).await; + tokio::fs::write(site.path().join("m.py"), b"x = 1\n") + .await + .unwrap(); + std::fs::set_permissions(&cache, std::fs::Permissions::from_mode(0o555)).unwrap(); + + apply_file_patch_at( + site.path(), + "m.py", + patched, + &compute_git_sha256_from_bytes(patched), + ) + .await + .unwrap(); + + let mode = std::fs::metadata(&cache).unwrap().permissions().mode() & 0o777; + std::fs::set_permissions(&cache, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert!(!cache.join("m.cpython-312.pyc").exists()); + assert_eq!(mode, 0o555, "the cache dir mode must be restored"); + } } diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 0c7177d49..17077daaf 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -3077,4 +3077,56 @@ mod tests { } assert_eq!(std::fs::read(member.join("index.js")).unwrap(), patched); } + + /// #819: rolling a modified `.py` file back to its original bytes + /// removes the bytecode compiled from the patched source, or a + /// same-size patch rolled back in the same second keeps running. + #[tokio::test] + async fn test_rollback_modified_py_removes_stale_bytecode() { + let site = tempfile::tempdir().unwrap(); + let blobs = tempfile::tempdir().unwrap(); + let original = b"AUTHOR = 'original'\n"; + let patched = b"AUTHOR = 'patchedX'\n"; + let before_hash = compute_git_sha256_from_bytes(original); + let after_hash = compute_git_sha256_from_bytes(patched); + tokio::fs::write(blobs.path().join(&before_hash), original) + .await + .unwrap(); + let cache = site.path().join("pkg/__pycache__"); + tokio::fs::create_dir_all(&cache).await.unwrap(); + tokio::fs::write(site.path().join("pkg/mod.py"), patched) + .await + .unwrap(); + tokio::fs::write(cache.join("mod.cpython-311.pyc"), b"pyc") + .await + .unwrap(); + tokio::fs::write(cache.join("other.cpython-311.pyc"), b"pyc") + .await + .unwrap(); + let mut files = HashMap::new(); + files.insert( + "pkg/mod.py".to_string(), + PatchFileInfo { + before_hash, + after_hash, + }, + ); + + let result = rollback_package_patch( + "pkg:pypi/pkg@1.0.0", + site.path(), + &files, + blobs.path(), + false, + ) + .await; + + assert!(result.success, "{:?}", result.error); + assert_eq!( + std::fs::read(site.path().join("pkg/mod.py")).unwrap(), + original + ); + assert!(!cache.join("mod.cpython-311.pyc").exists()); + assert!(cache.join("other.cpython-311.pyc").exists()); + } } From ee5e038ac149d7beca5ace558afe8515eea74a41 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 08:35:22 +0000 Subject: [PATCH 3/7] Drop stale Python bytecode when rewriting a .py Agent-mode apply and rollback now delete the __pycache__ bytecode (and a legacy sibling .pyc/.pyo) compiled from each .py file they rewrite. Before, a same-size patch applied in the same second as the install left the timestamp .pyc valid, so Python kept running the unpatched code while apply reported success and VEX attested not_affected. Rollback could keep the patched code running the same way. Python recompiles the bytecode on the next import. The helper rollback used for patch-added modules moves into the shared write path so both use one rule (#819). Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/e2e_pypi_bytecode.rs | 12 ++- crates/socket-patch-core/src/patch/apply.rs | 76 ++++++++++++++++++- .../socket-patch-core/src/patch/rollback.rs | 41 +--------- 3 files changed, 85 insertions(+), 44 deletions(-) diff --git a/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs b/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs index bee8d6fc3..8fa7f3fec 100644 --- a/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs +++ b/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs @@ -164,7 +164,10 @@ fn same_second_same_size_apply_and_rollback_change_imported_code() { }; let site = PathBuf::from(python( &py, - &["-c", "import sysconfig; print(sysconfig.get_paths()['purelib'])"], + &[ + "-c", + "import sysconfig; print(sysconfig.get_paths()['purelib'])", + ], )); // Install dupkit 1.0.0 the way pip leaves it: dist-info, module, and @@ -181,7 +184,12 @@ fn same_second_same_size_apply_and_rollback_change_imported_code() { std::fs::write(&module, ORIGINAL).unwrap(); python( &py, - &["-m", "compileall", "-q", module.parent().unwrap().to_str().unwrap()], + &[ + "-m", + "compileall", + "-q", + module.parent().unwrap().to_str().unwrap(), + ], ); assert_eq!(imported_version(&py), "original"); stage_patch(&project); diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 2bd800e3e..5a53709ba 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -1,8 +1,6 @@ use std::borrow::Cow; use std::collections::HashMap; -use std::path::Path; -#[cfg(unix)] -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use crate::hash::git_sha256::compute_git_sha256_from_bytes; use crate::manifest::schema::PatchFileInfo; @@ -554,6 +552,10 @@ pub(crate) async fn apply_file_patch_at( dir_guard.restore().await; write_result?; + // A rewritten `.py` file can't be trusted to invalidate its bytecode + // (#819): drop it so the next import compiles the new source. + remove_stale_bytecode(&filepath).await; + // Restore (or set) the final permissions on the post-rename inode. // On Unix this includes chown back to the pre-patch uid/gid (or // to the parent dir's uid/gid for new files); on Windows we only @@ -561,6 +563,74 @@ pub(crate) async fn apply_file_patch_at( restore_file_permissions(&filepath, existing_meta.as_ref()).await } +/// Delete the bytecode CPython compiled from the `.py` file `source`: the +/// `.[.opt-N].pyc` files in the sibling `__pycache__/`, and a +/// legacy (Python 2) `.pyc` / `.pyo` beside it. A no-op for +/// any other file. +/// +/// A timestamp-validated `.pyc` records only the source's mtime in whole +/// seconds and its size, so after a same-size rewrite in the same second +/// it still matches and the old code keeps running (#819). An +/// unchecked-hash `.pyc` is never revalidated at all. Bytecode is a cache: +/// the next import recompiles it from the source on disk. +/// +/// Only regular files are removed (never through a symlink), a read-only +/// directory is relaxed for the delete and put back, and failures are +/// ignored: the source write has already committed. +pub(crate) async fn remove_stale_bytecode(source: &Path) { + if !source.extension().is_some_and(|ext| ext == "py") { + return; + } + let (Some(stem), Some(dir)) = (source.file_stem().and_then(|s| s.to_str()), source.parent()) + else { + return; + }; + let legacy = ["pyc", "pyo"].map(|ext| dir.join(format!("{stem}.{ext}"))); + remove_regular_files(dir, &legacy).await; + + let cache = dir.join("__pycache__"); + match tokio::fs::symlink_metadata(&cache).await { + Ok(meta) if meta.is_dir() => {} + _ => return, + } + let Ok(mut entries) = tokio::fs::read_dir(&cache).await else { + return; + }; + let prefix = format!("{stem}."); + let mut stale = Vec::new(); + while let Ok(Some(entry)) = entries.next_entry().await { + let name = entry.file_name(); + if name + .to_str() + .is_some_and(|name| name.starts_with(&prefix) && name.ends_with(".pyc")) + { + stale.push(entry.path()); + } + } + remove_regular_files(&cache, &stale).await; +} + +/// Remove each of `paths` (all directly in `dir`) that is a regular file. +async fn remove_regular_files(dir: &Path, paths: &[PathBuf]) { + let mut doomed = Vec::new(); + for path in paths { + if tokio::fs::symlink_metadata(path) + .await + .is_ok_and(|meta| meta.is_file()) + { + doomed.push(path); + } + } + if doomed.is_empty() { + return; + } + let guard = DirWriteGuard::acquire(Some(dir)).await; + for path in doomed { + let _ = tokio::fs::remove_file(path).await; + } + guard.restore().await; +} + /// Guard that temporarily grants owner-write on a directory so the /// stage+rename write path can create and move files inside it, then /// restores the directory's original mode. diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 17077daaf..9e1c35714 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -516,11 +516,8 @@ async fn prune_emptied_parents(pkg_path: &Path, filepath: &Path) { } } if filepath.extension().is_some_and(|ext| ext == "py") { - if let Some(stem) = filepath.file_stem().and_then(|s| s.to_str()) { - let cache = filepath.with_file_name("__pycache__"); - remove_stale_bytecode(&cache, stem).await; - remove_dir_if_empty(&cache).await; - } + crate::patch::apply::remove_stale_bytecode(filepath).await; + remove_dir_if_empty(&filepath.with_file_name("__pycache__")).await; } for dir in chain.iter().rev() { if !remove_dir_if_empty(dir).await { @@ -529,40 +526,6 @@ async fn prune_emptied_parents(pkg_path: &Path, filepath: &Path) { } } -/// Delete the regular `.*.pyc` files in `cache`: bytecode compiled -/// from a module the rollback just removed. -async fn remove_stale_bytecode(cache: &Path, stem: &str) { - match tokio::fs::symlink_metadata(cache).await { - Ok(meta) if meta.is_dir() => {} - _ => return, - } - let Ok(mut entries) = tokio::fs::read_dir(cache).await else { - return; - }; - let prefix = format!("{stem}."); - let mut stale = Vec::new(); - while let Ok(Some(entry)) = entries.next_entry().await { - let name = entry.file_name(); - let Some(name) = name.to_str() else { - continue; - }; - if name.starts_with(&prefix) - && name.ends_with(".pyc") - && entry.file_type().await.is_ok_and(|t| t.is_file()) - { - stale.push(entry.path()); - } - } - if stale.is_empty() { - return; - } - let guard = crate::patch::apply::DirWriteGuard::acquire(Some(cache)).await; - for path in stale { - let _ = tokio::fs::remove_file(path).await; - } - guard.restore().await; -} - /// Remove `dir` if it is a real (non-symlink) empty directory. Returns /// whether it is gone. async fn remove_dir_if_empty(dir: &Path) -> bool { From c1a3367e9a635ae800193b02a35081eb9fbd6149 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 08:37:43 +0000 Subject: [PATCH 4/7] Satisfy clippy in the bytecode helper Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/patch/apply.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 5a53709ba..7fb26fdd4 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -578,7 +578,7 @@ pub(crate) async fn apply_file_patch_at( /// directory is relaxed for the delete and put back, and failures are /// ignored: the source write has already committed. pub(crate) async fn remove_stale_bytecode(source: &Path) { - if !source.extension().is_some_and(|ext| ext == "py") { + if source.extension().is_none_or(|ext| ext != "py") { return; } let (Some(stem), Some(dir)) = (source.file_stem().and_then(|s| s.to_str()), source.parent()) From d37916269d048ed82e145906e613692676d8ae0f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 08:53:46 +0000 Subject: [PATCH 5/7] Harden the stale bytecode cleanup Address review findings on the #819 fix: - A re-run of apply over a module an older release already patched now drops bytecode that predates the patched source, so upgrading and re-running apply repairs it. - A .pyc that can't be deleted is reported as a warning instead of being ignored. - Sourceless .pyc files beside the source are left alone: under Python 3 a package may ship them, and rollback could not bring them back. - Only exact .[.opt-N].pyc names match, so a dotted sibling module's bytecode is kept. - Rollback drops a deleted module's bytecode even when it can't prune the directories above it. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/patch/apply.rs | 242 ++++++++++++++---- .../socket-patch-core/src/patch/rollback.rs | 11 +- 2 files changed, 194 insertions(+), 59 deletions(-) diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 7fb26fdd4..9d368684a 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -554,81 +554,126 @@ pub(crate) async fn apply_file_patch_at( // A rewritten `.py` file can't be trusted to invalidate its bytecode // (#819): drop it so the next import compiles the new source. - remove_stale_bytecode(&filepath).await; + let bytecode_warning = remove_stale_bytecode(&filepath, StaleBytecode::All).await; // Restore (or set) the final permissions on the post-rename inode. // On Unix this includes chown back to the pre-patch uid/gid (or // to the parent dir's uid/gid for new files); on Windows we only // manage the readonly attribute. - restore_file_permissions(&filepath, existing_meta.as_ref()).await + let owner_warning = restore_file_permissions(&filepath, existing_meta.as_ref()).await?; + let warnings: Vec = owner_warning.into_iter().chain(bytecode_warning).collect(); + Ok((!warnings.is_empty()).then(|| warnings.join("; "))) } -/// Delete the bytecode CPython compiled from the `.py` file `source`: the -/// `.[.opt-N].pyc` files in the sibling `__pycache__/`, and a -/// legacy (Python 2) `.pyc` / `.pyo` beside it. A no-op for -/// any other file. +/// Which `__pycache__` entries of a module [`remove_stale_bytecode`] +/// deletes. +#[derive(Clone, Copy)] +pub(crate) enum StaleBytecode { + /// Every `.pyc` of the module: its source was just rewritten (or + /// deleted), so none of them was compiled from what is on disk now. + All, + /// Only `.pyc` files no newer than the source: compiled before the + /// source was last written, so possibly from older bytes. Bytecode + /// compiled after it (the first import following an earlier apply) + /// is kept. + NotNewerThanSource, +} + +/// Delete the `__pycache__/.[.opt-N].pyc` bytecode CPython +/// compiled from the `.py` file `source`. A no-op for any other file. /// /// A timestamp-validated `.pyc` records only the source's mtime in whole /// seconds and its size, so after a same-size rewrite in the same second -/// it still matches and the old code keeps running (#819). An +/// it still matches and the old code keeps running (#819); an /// unchecked-hash `.pyc` is never revalidated at all. Bytecode is a cache: -/// the next import recompiles it from the source on disk. +/// the next import recompiles it from the source on disk. A `.pyc` +/// beside the source is not touched: under Python 3 that is sourceless +/// bytecode a package may ship, not a cache. /// -/// Only regular files are removed (never through a symlink), a read-only -/// directory is relaxed for the delete and put back, and failures are -/// ignored: the source write has already committed. -pub(crate) async fn remove_stale_bytecode(source: &Path) { +/// Only regular files are removed (never through a symlink), and a +/// read-only `__pycache__` is relaxed for the delete and put back. Returns +/// a warning when a stale file could not be removed; the source write has +/// already committed either way. +pub(crate) async fn remove_stale_bytecode(source: &Path, which: StaleBytecode) -> Option { if source.extension().is_none_or(|ext| ext != "py") { - return; + return None; } let (Some(stem), Some(dir)) = (source.file_stem().and_then(|s| s.to_str()), source.parent()) else { - return; + return None; + }; + let cutoff = match which { + StaleBytecode::All => None, + StaleBytecode::NotNewerThanSource => { + match tokio::fs::metadata(source).await.and_then(|m| m.modified()) { + Ok(mtime) => Some(mtime), + Err(_) => return None, + } + } }; - let legacy = ["pyc", "pyo"].map(|ext| dir.join(format!("{stem}.{ext}"))); - remove_regular_files(dir, &legacy).await; - let cache = dir.join("__pycache__"); match tokio::fs::symlink_metadata(&cache).await { Ok(meta) if meta.is_dir() => {} - _ => return, + _ => return None, } let Ok(mut entries) = tokio::fs::read_dir(&cache).await else { - return; + return None; }; - let prefix = format!("{stem}."); let mut stale = Vec::new(); while let Ok(Some(entry)) = entries.next_entry().await { - let name = entry.file_name(); - if name + if !entry + .file_name() .to_str() - .is_some_and(|name| name.starts_with(&prefix) && name.ends_with(".pyc")) + .is_some_and(|name| is_bytecode_of(name, stem)) { - stale.push(entry.path()); + continue; } - } - remove_regular_files(&cache, &stale).await; -} - -/// Remove each of `paths` (all directly in `dir`) that is a regular file. -async fn remove_regular_files(dir: &Path, paths: &[PathBuf]) { - let mut doomed = Vec::new(); - for path in paths { - if tokio::fs::symlink_metadata(path) - .await - .is_ok_and(|meta| meta.is_file()) - { - doomed.push(path); + // `DirEntry::metadata` does not follow a symlink. + let Ok(meta) = entry.metadata().await else { + continue; + }; + let newer = cutoff.is_some_and(|cutoff| meta.modified().is_ok_and(|m| m > cutoff)); + if meta.is_file() && !newer { + stale.push(entry.path()); } } - if doomed.is_empty() { - return; + if stale.is_empty() { + return None; } - let guard = DirWriteGuard::acquire(Some(dir)).await; - for path in doomed { - let _ = tokio::fs::remove_file(path).await; + let guard = DirWriteGuard::acquire(Some(&cache)).await; + let mut failed = Vec::new(); + for path in stale { + if let Err(e) = tokio::fs::remove_file(&path).await { + if !is_missing_path(&e) { + failed.push(format!("{}: {e}", path.display())); + } + } } guard.restore().await; + (!failed.is_empty()).then(|| { + format!( + "could not remove stale Python bytecode ({}); Python may keep running \ + the old code until it is deleted", + failed.join(", ") + ) + }) +} + +/// Whether `name` is CPython's cache file for module `stem`: +/// `..pyc` or `..opt-.pyc` (PEP 3147 / PEP 488), +/// and not the bytecode of a dotted sibling such as `.x.py`. +fn is_bytecode_of(name: &str, stem: &str) -> bool { + let Some(rest) = name + .strip_prefix(stem) + .and_then(|rest| rest.strip_prefix('.')) + .and_then(|rest| rest.strip_suffix(".pyc")) + else { + return false; + }; + match rest.split_once('.') { + None => !rest.is_empty(), + Some((tag, opt)) => !tag.is_empty() && opt.starts_with("opt-") && !opt.contains('.'), + } } /// Guard that temporarily grants owner-write on a directory so the @@ -1025,6 +1070,23 @@ async fn apply_package_patch_at( result.files_verified.push(verify_result); } + // A file an earlier run already patched may still have bytecode + // compiled from the unpatched source beside it (written by a release + // without the #819 fix, or a run that stopped between the rename and + // the cleanup). Drop whatever predates the source. + if !dry_run { + for v in &result.files_verified { + if v.status == VerifyStatus::AlreadyPatched { + let source = pkg_path.join(normalize_file_path(&v.file)); + if let Some(warning) = + remove_stale_bytecode(&source, StaleBytecode::NotNewerThanSource).await + { + result.error = Some(warning); + } + } + } + } + // Check if all files are already patched let all_already_patched = result .files_verified @@ -1106,7 +1168,7 @@ async fn apply_package_patch_at( // Advisory notes from writes that committed but could not fully restore // metadata (see `apply_file_patch_at`); reported on `error` alongside // `success` — the success-with-note shape the `--force` skip uses. - let mut warnings: Vec = Vec::new(); + let mut warnings: Vec = result.error.take().into_iter().collect(); // Apply patches to files that need it. for (file_name, file_info) in files_in_order(files) { @@ -3596,7 +3658,8 @@ mod tests { /// from the old source. A timestamp `.pyc` only records the source's /// mtime (whole seconds) and size, so a same-size patch in the same /// second as the install would otherwise keep the unpatched bytecode - /// running. Bytecode for other modules stays. + /// running. Bytecode for other modules (a dotted `mod.x.py` too) and + /// sourceless `.pyc` files beside the source stay. #[tokio::test] async fn test_apply_file_patch_py_removes_stale_bytecode() { let site = tempfile::tempdir().unwrap(); @@ -3611,9 +3674,9 @@ mod tests { "pkg/__pycache__/mod.cpython-313.opt-2.pyc", "pkg/__pycache__/modx.cpython-311.pyc", "pkg/__pycache__/other.cpython-311.pyc", + "pkg/__pycache__/mod.x.cpython-311.pyc", + "pkg/__pycache__/mod.x.cpython-311.opt-1.pyc", "pkg/mod.pyc", - "pkg/mod.pyo", - "pkg/other.pyc", ], ) .await; @@ -3636,15 +3699,15 @@ mod tests { "__pycache__/mod.cpython-311.pyc", "__pycache__/mod.cpython-311.opt-1.pyc", "__pycache__/mod.cpython-313.opt-2.pyc", - "mod.pyc", - "mod.pyo", ] { assert!(!pkg.join(stale).exists(), "{stale} must be removed"); } for kept in [ "__pycache__/modx.cpython-311.pyc", "__pycache__/other.cpython-311.pyc", - "other.pyc", + "__pycache__/mod.x.cpython-311.pyc", + "__pycache__/mod.x.cpython-311.opt-1.pyc", + "mod.pyc", ] { assert!(pkg.join(kept).exists(), "{kept} must be kept"); } @@ -3679,11 +3742,7 @@ mod tests { async fn test_apply_file_patch_non_py_keeps_bytecode() { let site = tempfile::tempdir().unwrap(); let patched = b"patched"; - touch( - site.path(), - &["pkg/__pycache__/mod.cpython-311.pyc", "pkg/mod.pyc"], - ) - .await; + touch(site.path(), &["pkg/__pycache__/mod.cpython-311.pyc"]).await; for name in ["pkg/mod.js", "pkg/mod.pyi"] { tokio::fs::write(site.path().join(name), b"original") .await @@ -3702,7 +3761,6 @@ mod tests { .path() .join("pkg/__pycache__/mod.cpython-311.pyc") .exists()); - assert!(site.path().join("pkg/mod.pyc").exists()); } /// A read-only `__pycache__` (a root-owned system site-packages, or a @@ -3735,4 +3793,78 @@ mod tests { assert!(!cache.join("m.cpython-312.pyc").exists()); assert_eq!(mode, 0o555, "the cache dir mode must be restored"); } + + /// #819: a re-run over a module an earlier release already patched + /// (without dropping its bytecode) removes the bytecode that predates + /// the patched source, and keeps bytecode compiled after it. + #[tokio::test] + async fn test_apply_already_patched_py_removes_older_bytecode() { + use std::time::{Duration, SystemTime}; + let site = tempfile::tempdir().unwrap(); + let blobs = tempfile::tempdir().unwrap(); + let patched = b"x = 2\n"; + let source = site.path().join("pkg/mod.py"); + let cache = site.path().join("pkg/__pycache__"); + touch( + site.path(), + &[ + "pkg/__pycache__/mod.cpython-311.pyc", + "pkg/__pycache__/mod.cpython-312.pyc", + ], + ) + .await; + tokio::fs::write(&source, patched).await.unwrap(); + let mtime = SystemTime::now() - Duration::from_secs(60); + let set = |path: &Path, t: SystemTime| { + std::fs::File::options() + .write(true) + .open(path) + .unwrap() + .set_modified(t) + .unwrap(); + }; + set(&source, mtime); + set( + &cache.join("mod.cpython-311.pyc"), + mtime - Duration::from_secs(1), + ); + set( + &cache.join("mod.cpython-312.pyc"), + mtime + Duration::from_secs(1), + ); + let mut files = HashMap::new(); + files.insert( + "pkg/mod.py".to_string(), + PatchFileInfo { + before_hash: compute_git_sha256_from_bytes(b"x = 1\n"), + after_hash: compute_git_sha256_from_bytes(patched), + }, + ); + + let result = apply_package_patch( + "pkg:pypi/pkg@1.0.0", + site.path(), + &files, + &PatchSources::blobs_only(blobs.path()), + false, + MismatchPolicy::Warn, + ) + .await; + + assert!(result.success, "{:?}", result.error); + assert!(result.files_patched.is_empty()); + assert!(!cache.join("mod.cpython-311.pyc").exists()); + assert!(cache.join("mod.cpython-312.pyc").exists()); + } + + #[test] + fn test_is_bytecode_of() { + assert!(is_bytecode_of("six.cpython-311.pyc", "six")); + assert!(is_bytecode_of("six.pypy39.opt-1.pyc", "six")); + assert!(!is_bytecode_of("six.pyc", "six")); + assert!(!is_bytecode_of("sixx.cpython-311.pyc", "six")); + assert!(!is_bytecode_of("six.moves.cpython-311.pyc", "six")); + assert!(!is_bytecode_of("six.cpython-311.opt-1.x.pyc", "six")); + assert!(!is_bytecode_of("six.cpython-311.pyo", "six")); + } } diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 9e1c35714..a78820f2c 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -497,6 +497,13 @@ impl crate::patch::store_copies::CopyFold for RollbackResult { /// Best effort: the file delete already committed the rollback, so a /// directory that can't be removed is left in place rather than failing it. async fn prune_emptied_parents(pkg_path: &Path, filepath: &Path) { + // The bytecode goes whatever the pruning below decides: a stale + // `.pyc` of a deleted module must not outlive it. + if filepath.extension().is_some_and(|ext| ext == "py") { + use crate::patch::apply::{remove_stale_bytecode, StaleBytecode}; + remove_stale_bytecode(filepath, StaleBytecode::All).await; + remove_dir_if_empty(&filepath.with_file_name("__pycache__")).await; + } let Some(rel) = filepath .parent() .and_then(|parent| parent.strip_prefix(pkg_path).ok()) @@ -515,10 +522,6 @@ async fn prune_emptied_parents(pkg_path: &Path, filepath: &Path) { _ => return, } } - if filepath.extension().is_some_and(|ext| ext == "py") { - crate::patch::apply::remove_stale_bytecode(filepath).await; - remove_dir_if_empty(&filepath.with_file_name("__pycache__")).await; - } for dir in chain.iter().rev() { if !remove_dir_if_empty(dir).await { return; From a98b93d2677eb39cf5884941c28de126584e8c5c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 11:14:38 +0000 Subject: [PATCH 6/7] Report stale bytecode cleanup failures A .pyc that apply or rollback could not delete was put on the result as a note, but the CLI only surfaced ownership notes: apply dropped it, and rollback labelled it ownership_not_restored. Map each advisory note to its warning code by marker, so the failure shows up as stale_bytecode_not_removed. Notes from several already patched files are now all kept instead of only the last, and a --force skip message no longer replaces them. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 4 +- crates/socket-patch-cli/src/commands/apply.rs | 11 +-- .../socket-patch-cli/src/commands/rollback.rs | 23 ++++--- crates/socket-patch-core/src/patch/apply.rs | 67 +++++++++++++++---- 4 files changed, 77 insertions(+), 28 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index e2bc9b3d9..002f6c244 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -283,7 +283,7 @@ For a **9.0 root lock**, the CLI ensures `pnpm-workspace.yaml` carries `trustLoc **Hosted-state visibility (`redirectState`, additive/MINOR).** Every non-hosted-mode, non-vendored-mode `scan --json` SUCCESS envelope (report-only, `--mode agent`/`--sync`, and the zero-discovery envelope) carries an additive top-level `redirectState` object whenever the project's lockfiles pin ≥ 1 hosted patch: `{ mode, records: [{purl, uuid}], wiringLive: [purl] }`. It is a descriptive STATE block, not a warning. `mode` is the constant `"hosted"`. **v5.0 (MAJOR shape change)**: hosted mode keeps no ledger, so `records` lists the hosted pins lockfile discovery finds (one per `(purl, uuid)`, the same discovery `vex` uses: a hosted URL counts only on `https://patch.socket.dev` or the `--patch-server-url` origin), and the v4 `ledger` and `records[].ledgerKey` keys are gone. Each record's `purl` is CANONICALIZED (qualifiers stripped, percent-decoded — e.g. `pkg:npm/@scope/pkg@1.0.0`, `pkg:gem/nokogiri@1.13.3`) to the same spelling `wiringLive` carries, so the join is a plain string compare. `wiringLive` is the subset of those pins among this run's *counted* purls (post-`--ecosystems`-filter) — computed once per run, the same set that feeds `hosted_wiring_retained`. A record missing from `wiringLive` is still wired; it just was not crawled/queried this run (an `--ecosystems` filter, a zero discovery). The key is omitted when no lockfile pins a hosted patch (and under `--global`), and error envelopes (the `--offline` refusal, all-batches-failed) are deliberately minimal and never carry it. A pre-v5 `.socket/vendor/redirect-state.json` is not read. Hosted-mode runs carry their own `redirect` payload and `details.mode: "hosted"` events instead, and vendored-mode runs carry the takeover warnings (their takeovers may restore pins mid-run) — neither duplicates a pre-run snapshot that could go stale. -**Agent-flow run-level warnings (additive).** An agent-mode apply (`--mode agent` / `--sync`, `--json`) may add a top-level `warnings[]` array of `{code, detail}` entries to the scan envelope (absent when none fired; each is also mirrored to stderr unless `--silent`). They surface cross-mode state the apply cannot change — never a status or exit-code change (hosted refusals set the precedent: exit 0 + warning). Codes (stable; new codes are additive/MINOR): `vendored_ownership_retained` — vendor-owned package(s) were skipped before download (each is also a `skipped` / `vendored` event); the detail names the purls and the migration path (`remove `, or `vendor --revert` which unwinds every vendored package, then re-run). `hosted_wiring_retained` — the lockfiles still pin scanned package(s) to a hosted patch (the agent run does not unwind hosted wiring — as of v5.0 that is `socket-patch rollback`'s job, which restores the upstream registry entries, or `remove ` per package); the detail names the purls and the options (stay `--mode hosted`, migrate via `scan --mode vendored`, or `socket-patch rollback`). The warning keys on the hosted pins lockfile discovery finds at scan time, so a flow that restored the upstream entries retires it. The human path prints the same `hosted_wiring_retained` text to stderr after an apply; the vendored counterpart is already covered by its per-package `[skip] … (vendored …)` lines. `ownership_not_restored` (v5.0; `apply` and `rollback` `warnings[]` alike) — a file WAS patched (or restored) but its ownership could not be put back to the original uid/gid (the mode is still restored last); the detail is `: : patched, but ownership could not be restored to uid N gid M: ` and the human line `Warning: ` (stderr, muted by `--silent`); never a status or exit change. `cargo_build_cache_stale` (v5.0; `apply` and `rollback` `warnings[]` alike) — a cargo crate's bytes changed but its compiled copy in a project build directory could not be invalidated (a fingerprint directory that could not be removed, or a `build.build-dir` with a `{workspace-path-hash}` template); the detail names the crates to `cargo clean -p` before the next build, which may otherwise link the stale code; never a status or exit change. +**Agent-flow run-level warnings (additive).** An agent-mode apply (`--mode agent` / `--sync`, `--json`) may add a top-level `warnings[]` array of `{code, detail}` entries to the scan envelope (absent when none fired; each is also mirrored to stderr unless `--silent`). They surface cross-mode state the apply cannot change — never a status or exit-code change (hosted refusals set the precedent: exit 0 + warning). Codes (stable; new codes are additive/MINOR): `vendored_ownership_retained` — vendor-owned package(s) were skipped before download (each is also a `skipped` / `vendored` event); the detail names the purls and the migration path (`remove `, or `vendor --revert` which unwinds every vendored package, then re-run). `hosted_wiring_retained` — the lockfiles still pin scanned package(s) to a hosted patch (the agent run does not unwind hosted wiring — as of v5.0 that is `socket-patch rollback`'s job, which restores the upstream registry entries, or `remove ` per package); the detail names the purls and the options (stay `--mode hosted`, migrate via `scan --mode vendored`, or `socket-patch rollback`). The warning keys on the hosted pins lockfile discovery finds at scan time, so a flow that restored the upstream entries retires it. The human path prints the same `hosted_wiring_retained` text to stderr after an apply; the vendored counterpart is already covered by its per-package `[skip] … (vendored …)` lines. `ownership_not_restored` (v5.0; `apply` and `rollback` `warnings[]` alike) — a file WAS patched (or restored) but its ownership could not be put back to the original uid/gid (the mode is still restored last); the detail is `: : patched, but ownership could not be restored to uid N gid M: ` and the human line `Warning: ` (stderr, muted by `--silent`); never a status or exit change. `stale_bytecode_not_removed` (`apply` and `rollback` `warnings[]` alike) — a `.py` file WAS patched (or restored) but CPython bytecode compiled from its previous bytes (`__pycache__/.[.opt-N].pyc`) could not be deleted, so Python may keep running the old code until it is removed; the detail is `: could not remove stale Python bytecode (: , …); …` and the human line `Warning: `; never a status or exit change. `cargo_build_cache_stale` (v5.0; `apply` and `rollback` `warnings[]` alike) — a cargo crate's bytes changed but its compiled copy in a project build directory could not be invalidated (a fingerprint directory that could not be removed, or a `build.build-dir` with a `{workspace-path-hash}` template); the detail names the crates to `cargo clean -p` before the next build, which may otherwise link the stale code; never a status or exit change. `scan --prune` opts into garbage collection. When set, `scan` removes manifest entries for packages no longer present in the crawl, then deletes orphan blob files, and every obsolete diff and package archive, from `.socket/`. A blob is an orphan when no patch left in the manifest references it: the afterHash and beforeHash blobs of every remaining patch are kept, the same retention policy `repair` uses (the beforeHash blobs are an offline rollback's only restore data, and `repair` downloads afterHash blobs only). Diff and legacy package archives are never kept, even for a remaining patch: nothing reads them any more. Off by default (v3.0) so a temporary uninstall doesn't silently destroy manifest state. Only entries whose ecosystem this run actually crawled are eligible: a `pkg:/` with no crawler in this build (a newer CLI's ecosystem in the committed manifest) is exempt — the crawl never looked for them, so their absence is not evidence of removal (same fail-safe as the `--ecosystems` filter, which narrows the query but never the prune's installed set). A Cargo entry is also exempt while its agent-mode copy is still patched: the project crawl looks up only the crates `Cargo.lock` resolves, but a crate the lock bumped or dropped keeps its patched copy in the machine-wide `$CARGO_HOME/registry/src` cache (nothing deletes it), and the entry holds the only blobs that can restore that copy. The wet pass keeps it with a `cargo_cache_patch_kept` warning naming the `socket-patch rollback ` that restores the copy and drops the entry; the `--dry-run` preview reports no event (and no warning) for it. The pass also reconciles vendored state (runs FIRST, under ONE apply-lock acquisition shared with the manifest prune — lock contention skips the whole pass without failing the scan; `--lock-timeout` is honored and a lock I/O error is reported rather than swallowed; the existence gate — a manifest file OR a vendor ledger file, both cheap stats; an emptied ledger is deleted on save, so its presence is its content proxy — runs BEFORE the lock, so a bare project never gets a `.socket/`; in the vendored scan arms the pass runs AFTER the vendor step): (a) ledger entries still tracked by a manifest record (manifest-mode entries written by standalone `vendor`) whose patch is gone from the manifest are reverted — `detached` entries (every `scan`/`get --mode vendored` entry, v5.0) have no manifest record to lose and are exempt from this leg; (b) EVERY ledger entry whose dependency is no longer in the lockfile graph is reverted and any manifest entry it still had dropped (v5.0: the check is about the lockfile, not the manifest, so embedded-record entries are no longer exempt; a missing or undeterminable lockfile keeps the entry, fail-safe); and (c) orphan `.socket/vendor//` dirs with no ledger entry are swept. The prune never deletes a zero-patch `.socket/manifest.json` (its `{"patches": {}}` + `setup` block stay). Under `--json` (v5.0, MAJOR) the pass is recorded into the scan envelope: each pruned manifest entry is a `removed` event with `details.manifest: true`, each reverted vendored entry a `removed` event (`errorCode: "vendor_reverted"`, `details.mode: "vendored"`), each drift-kept entry a `skipped` / `vendor_revert_kept` event and each entry whose revert failed a `skipped` / `vendor_revert_failed` event (a GC revert failure never fails the run); a `--dry-run` preview reports the removals as `verified`. The artifact sweep is the envelope's `gc` (`summary.bytesFreed` mirrors it) and the orphan vendor dirs swept are the top-level `removedVendorOrphanDirs` count. The pass's warnings join the top-level `warnings[]`: `vendor_state_write_failed` / `manifest_write_failed` (entries were reverted but the ledger or manifest rewrite failed), `cleanup_failed` (an orphan sweep failed mid-way), `cargo_cache_patch_kept` (see above), and the reinstall advisories of the vendored reverts (`vendor_bun_reinstall_required`, `vendor_vlt_reinstall_required`, `vendor_pypi_reinstall_required`; a revert's other warnings, such as `vendor_lock_entry_removed` or a drift keep's, are not repeated). A pass skipped at the lock (`lock_held` | `lock_io`) has no `gc` and no events, only a `gc_skipped` warning whose detail ends with the code. Human mode prints `GC: skipped: .`, one `GC: .` line per warning, and `GC: failed to revert N vendored entries: …` (singular for one). A drift keep is an entry the revert deliberately preserved (`vendor_artifact_kept` — undo the drift and re-run `vendor --revert` to finish); the preview cannot see drift (backends return before the wiring replay on dry runs), so its `verified` reverts may over-promise what a wet run will actually reclaim. @@ -1190,7 +1190,7 @@ v5.0 replaces v4's per-purl reverts and whole-ledger reverse replay (`revert_rem | Hosted leg: pin refused / write failed / contested wiring | `failed` `hosted_restore_refused` / artifact-level `hosted_write_failed` / artifact-level `hosted_wiring_contested`, `details.mode: "hosted"` | | Manifest entry dropped | `removed` (`verified` on `--dry-run`) with `details.manifest: true` | -Events carry the patch `uuid` when one is recorded. A package wired through two legs gets one event per leg. Cargo's `.cargo-checksum.json` resync rides the envelope's `sidecars[]`. Rollback's own top-level keys: `hosted: {editedFiles: N}` (distinct files the hosted leg rewrote) and `paths` (the path-glob targets verbatim, `[]` when none); `path` on `manifest_not_found` and `legacyRedirectLedgerRemoved` on the pre-v5-ledger exit. `gc` is the shared GC object (via `summary.bytesFreed` too); it is absent under `--preserve-state`, and absent with a `gc_skipped` warning after a before-blob gate abort or under a corrupt vendor ledger. `warnings[]` (omitted when empty): `reinstall_required`, `hosted_state_not_preservable`, `out_of_scope_copies_restored`, `vendor_state_unreadable`, `cleanup_failed`, `manifest_write_failed`, `gc_skipped`, `legacy_redirect_ledger_kept`, `ownership_not_restored`, `rollback_record_superseded`, `gradle_m2_copy_not_restored`, `cargo_build_cache_stale` (see the apply warnings), `vendor_orphan_still_wired`, the upstream-restore advisories (including `hosted_resolution_orphaned`) and the vendored/hosted leg advisories. +Events carry the patch `uuid` when one is recorded. A package wired through two legs gets one event per leg. Cargo's `.cargo-checksum.json` resync rides the envelope's `sidecars[]`. Rollback's own top-level keys: `hosted: {editedFiles: N}` (distinct files the hosted leg rewrote) and `paths` (the path-glob targets verbatim, `[]` when none); `path` on `manifest_not_found` and `legacyRedirectLedgerRemoved` on the pre-v5-ledger exit. `gc` is the shared GC object (via `summary.bytesFreed` too); it is absent under `--preserve-state`, and absent with a `gc_skipped` warning after a before-blob gate abort or under a corrupt vendor ledger. `warnings[]` (omitted when empty): `reinstall_required`, `hosted_state_not_preservable`, `out_of_scope_copies_restored`, `vendor_state_unreadable`, `cleanup_failed`, `manifest_write_failed`, `gc_skipped`, `legacy_redirect_ledger_kept`, `ownership_not_restored`, `stale_bytecode_not_removed`, `rollback_record_superseded`, `gradle_m2_copy_not_restored`, `cargo_build_cache_stale` (see the apply warnings), `vendor_orphan_still_wired`, the upstream-restore advisories (including `hosted_resolution_orphaned`) and the vendored/hosted leg advisories. **Status and exit**: `success` (exit 0) unless something leaves the system still patched — an agent failure, a vendored drift-keep or failure, a hosted refusal, contested wiring under an unscoped run, a corrupt vendor ledger, a failed manifest write — which is `partialFailure` (exit 1). When something failed and nothing was rolled back, previewed, already original or not installed, the run failed as a whole: `status: "error"` with `error: {code: "rollback_failed"}` (exit 1; the `failed` events stay). Not-installed entries never flip the exit (the apply/rollback asymmetry). GC failures never affect the exit. A dry run's would-be refusals stay `failed` events (not `skipped`): a dry run that cannot roll something back exits 1, as before. Every early error (`manifest_not_found`, `manifest_invalid`, `manifest_unreadable`, `patch_not_found`, `ambiguous_target`, `path_glob_no_match`, `hosted_wiring_contested`, `vendor_ledger_missing`, `lock_held` / `lock_io`, the catch-all `rollback_failed`, and the exit-2 usage error `path_glob_invalid`) is a full envelope with empty `events`. diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index 1f915eb2f..7385ac8c2 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -1363,11 +1363,12 @@ pub(crate) async fn run_locked( } else { Vec::new() }; - run_warnings.extend(results.iter().filter_map(|r| { - let note = r.error.as_deref().filter(|_| r.success)?; - note.contains(socket_patch_core::patch::apply::OWNERSHIP_NOT_RESTORED_MARKER) - .then(|| RunWarning { - code: "ownership_not_restored".to_string(), + run_warnings.extend(results.iter().flat_map(|r| { + let note = r.error.as_deref().filter(|_| r.success).unwrap_or_default(); + socket_patch_core::patch::apply::advisory_warning_codes(note) + .into_iter() + .map(move |code| RunWarning { + code: code.to_string(), detail: format!("{}: {note}", normalize_purl(&r.package_key)), }) })); diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 93b0929dd..acbee840a 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -2092,17 +2092,22 @@ pub async fn run(args: RollbackArgs) -> i32 { .iter() .chain(hosted_leg.warnings.iter()) .for_each(|(code, detail)| run_warnings.push((code.clone(), detail.clone()))); - // A restored package whose ownership could not be put back - // (the engine reports it on `error` with `success: true`) is - // restored but worth a note, here and in the envelope. + // A restored package whose ownership could not be put back, or + // whose stale bytecode could not be removed (the engine reports + // it on `error` with `success: true`), is restored but worth a + // note, here and in the envelope. for r in results.iter().filter(|r| r.success) { if let Some(note) = &r.error { - let warning = ( - "ownership_not_restored".to_string(), - format!("{}: {note}", r.package_key), - ); - human_warnings.push(warning.clone()); - run_warnings.push(warning); + let codes = socket_patch_core::patch::apply::advisory_warning_codes(note); + let codes = if codes.is_empty() { + vec!["ownership_not_restored"] + } else { + codes + }; + let detail = format!("{}: {note}", r.package_key); + // One human line per note, one envelope entry per code. + human_warnings.push((codes[0].to_string(), detail.clone())); + run_warnings.extend(codes.iter().map(|c| (c.to_string(), detail.clone()))); } } diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 9d368684a..b254da2a8 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -652,7 +652,7 @@ pub(crate) async fn remove_stale_bytecode(source: &Path, which: StaleBytecode) - guard.restore().await; (!failed.is_empty()).then(|| { format!( - "could not remove stale Python bytecode ({}); Python may keep running \ + "{STALE_BYTECODE_NOT_REMOVED_MARKER} ({}); Python may keep running \ the old code until it is deleted", failed.join(", ") ) @@ -951,6 +951,28 @@ impl crate::patch::store_copies::CopyFold for ApplyResult { /// keys on the same text. pub const OWNERSHIP_NOT_RESTORED_MARKER: &str = "ownership could not be restored"; +/// The substring every failed bytecode cleanup carries (see +/// [`remove_stale_bytecode`]); the CLI's `stale_bytecode_not_removed` +/// warning keys on the same text. +pub const STALE_BYTECODE_NOT_REMOVED_MARKER: &str = "could not remove stale Python bytecode"; + +/// The run-level warning codes a successful apply's or rollback's advisory +/// `note` (its `error` with `success: true`) carries, by marker. One note +/// can join several advisories with `"; "`. +pub fn advisory_warning_codes(note: &str) -> Vec<&'static str> { + [ + (OWNERSHIP_NOT_RESTORED_MARKER, "ownership_not_restored"), + ( + STALE_BYTECODE_NOT_REMOVED_MARKER, + "stale_bytecode_not_removed", + ), + ] + .into_iter() + .filter(|(marker, _)| note.contains(marker)) + .map(|(_, code)| code) + .collect() +} + /// The message of a new-file collision (see [`mark_new_file_collision`]). pub const NEW_FILE_COLLISION_MESSAGE: &str = "File already exists, but the patch adds it as a new file (its content matches neither hash)"; @@ -1074,18 +1096,19 @@ async fn apply_package_patch_at( // compiled from the unpatched source beside it (written by a release // without the #819 fix, or a run that stopped between the rename and // the cleanup). Drop whatever predates the source. + // Its warnings ride `error` (with `success: true`) on every exit below. + let mut bytecode_warnings: Vec = Vec::new(); if !dry_run { for v in &result.files_verified { if v.status == VerifyStatus::AlreadyPatched { let source = pkg_path.join(normalize_file_path(&v.file)); - if let Some(warning) = - remove_stale_bytecode(&source, StaleBytecode::NotNewerThanSource).await - { - result.error = Some(warning); - } + bytecode_warnings.extend( + remove_stale_bytecode(&source, StaleBytecode::NotNewerThanSource).await, + ); } } } + let join_notes = |notes: Vec| (!notes.is_empty()).then(|| notes.join("; ")); // Check if all files are already patched let all_already_patched = result @@ -1117,6 +1140,7 @@ async fn apply_package_patch_at( if all_already_patched { coursier_retry(&mut result); result.success = true; + result.error = join_notes(bytecode_warnings); return result; } @@ -1135,10 +1159,11 @@ async fn apply_package_patch_at( .filter(|v| v.status == VerifyStatus::NotFound) .count(); result.success = true; - result.error = Some(format!( + let skipped = format!( "All patch files were skipped: {} not found on disk (--force)", not_found_count - )); + ); + result.error = join_notes([skipped].into_iter().chain(bytecode_warnings).collect()); return result; } @@ -1168,7 +1193,7 @@ async fn apply_package_patch_at( // Advisory notes from writes that committed but could not fully restore // metadata (see `apply_file_patch_at`); reported on `error` alongside // `success` — the success-with-note shape the `--force` skip uses. - let mut warnings: Vec = result.error.take().into_iter().collect(); + let mut warnings: Vec = bytecode_warnings; // Apply patches to files that need it. for (file_name, file_info) in files_in_order(files) { @@ -1272,9 +1297,7 @@ async fn apply_package_patch_at( } } - if !warnings.is_empty() { - result.error = Some(warnings.join("; ")); - } + result.error = join_notes(warnings); result.success = true; result } @@ -3867,4 +3890,24 @@ mod tests { assert!(!is_bytecode_of("six.cpython-311.opt-1.x.pyc", "six")); assert!(!is_bytecode_of("six.cpython-311.pyo", "six")); } + + #[test] + fn test_advisory_warning_codes() { + let ownership = + format!("a.js: patched, but {OWNERSHIP_NOT_RESTORED_MARKER} to uid 1 gid 2: EPERM"); + let bytecode = format!("{STALE_BYTECODE_NOT_REMOVED_MARKER} (x.pyc: EACCES); Python may keep running the old code until it is deleted"); + assert_eq!( + advisory_warning_codes(&ownership), + ["ownership_not_restored"] + ); + assert_eq!( + advisory_warning_codes(&bytecode), + ["stale_bytecode_not_removed"] + ); + assert_eq!( + advisory_warning_codes(&format!("{ownership}; {bytecode}")), + ["ownership_not_restored", "stale_bytecode_not_removed"] + ); + assert!(advisory_warning_codes("All patch files were skipped").is_empty()); + } } From de736a0a1618d217d163b9191aafff34914c7b5a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 16:38:14 +0000 Subject: [PATCH 7/7] Report bytecode cleanup failures on rollback deletes Rolling back a patch-added .py file removed its stale bytecode but dropped the warning when a .pyc could not be deleted. It now joins the rollback result, so the CLI reports stale_bytecode_not_removed just as it does for restored files. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-core/src/patch/rollback.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index a78820f2c..f6b1e52ee 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -482,8 +482,9 @@ impl crate::patch::store_copies::CopyFold for RollbackResult { /// is back to its pre-apply shape. In site-packages a leftover empty /// directory still imports as a PEP 420 namespace package (#838). /// -/// For a `.py` file, its now-stale bytecode in the sibling `__pycache__/` -/// (`.[.opt-N].pyc`) goes first. Then each parent directory is +/// The caller removes a `.py` file's now-stale bytecode first (see +/// `remove_stale_bytecode`), so an emptied `__pycache__/` is pruned here +/// like any other directory. Each parent directory is /// removed while it is empty, deepest first, stopping at `pkg_path` /// (never removed) or at the first directory that still holds anything. /// Apply records no list of the directories it created, so "empty once @@ -497,11 +498,7 @@ impl crate::patch::store_copies::CopyFold for RollbackResult { /// Best effort: the file delete already committed the rollback, so a /// directory that can't be removed is left in place rather than failing it. async fn prune_emptied_parents(pkg_path: &Path, filepath: &Path) { - // The bytecode goes whatever the pruning below decides: a stale - // `.pyc` of a deleted module must not outlive it. if filepath.extension().is_some_and(|ext| ext == "py") { - use crate::patch::apply::{remove_stale_bytecode, StaleBytecode}; - remove_stale_bytecode(filepath, StaleBytecode::All).await; remove_dir_if_empty(&filepath.with_file_name("__pycache__")).await; } let Some(rel) = filepath @@ -670,6 +667,16 @@ async fn rollback_package_patch_at( result.error = Some(format!("Failed to delete {}: {}", file_name, e)); return result; } + // The bytecode goes whatever the pruning decides: a stale `.pyc` + // of a deleted module must not outlive it, and a failed delete is + // reported like a restore's (`stale_bytecode_not_removed`). + warnings.extend( + crate::patch::apply::remove_stale_bytecode( + &filepath, + crate::patch::apply::StaleBytecode::All, + ) + .await, + ); prune_emptied_parents(pkg_path, &filepath).await; result.files_rolled_back.push(file_name.clone()); continue;