diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 0d65d61fa..f62a5d5bb 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. `go_installed_tool_stale` (v5.0; `apply`, dry runs included) — a `--global` Go apply patched a module cache copy that a tool installed with `go install` was built from before the patch; the detail names the binary and the `go install @` remedy (or, when they can't be inspected, what could not be checked); 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. `go_installed_tool_stale` (v5.0; `apply`, dry runs included) — a `--global` Go apply patched a module cache copy that a tool installed with `go install` was built from before the patch; the detail names the binary and the `go install @` remedy (or, when they can't be inspected, what could not be checked); 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. @@ -1199,7 +1199,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`, `patched_in_other_scope`, `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`, `patched_in_other_scope`, `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 3d951a0d9..0780a3cfa 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 ee801ff23..2c0950c7e 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -2202,17 +2202,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-cli/tests/e2e_pypi_bytecode.rs b/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs new file mode 100644 index 000000000..8fa7f3fec --- /dev/null +++ b/crates/socket-patch-cli/tests/e2e_pypi_bytecode.rs @@ -0,0 +1,221 @@ +//! 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..b254da2a8 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,11 +552,128 @@ 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. + 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("; "))) +} + +/// 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 +/// unchecked-hash `.pyc` is never revalidated at all. Bytecode is a cache: +/// 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), 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 None; + } + let (Some(stem), Some(dir)) = (source.file_stem().and_then(|s| s.to_str()), source.parent()) + else { + 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 cache = dir.join("__pycache__"); + match tokio::fs::symlink_metadata(&cache).await { + Ok(meta) if meta.is_dir() => {} + _ => return None, + } + let Ok(mut entries) = tokio::fs::read_dir(&cache).await else { + return None; + }; + let mut stale = Vec::new(); + while let Ok(Some(entry)) = entries.next_entry().await { + if !entry + .file_name() + .to_str() + .is_some_and(|name| is_bytecode_of(name, stem)) + { + continue; + } + // `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 stale.is_empty() { + return None; + } + 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!( + "{STALE_BYTECODE_NOT_REMOVED_MARKER} ({}); 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 @@ -836,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)"; @@ -955,6 +1092,24 @@ 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. + // 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)); + 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 .files_verified @@ -985,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; } @@ -1003,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; } @@ -1036,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 = Vec::new(); + let mut warnings: Vec = bytecode_warnings; // Apply patches to files that need it. for (file_name, file_info) in files_in_order(files) { @@ -1140,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 } @@ -3510,4 +3665,249 @@ 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 (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(); + 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/__pycache__/mod.x.cpython-311.pyc", + "pkg/__pycache__/mod.x.cpython-311.opt-1.pyc", + "pkg/mod.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", + ] { + assert!(!pkg.join(stale).exists(), "{stale} must be removed"); + } + for kept in [ + "__pycache__/modx.cpython-311.pyc", + "__pycache__/other.cpython-311.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"); + } + } + + /// #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"]).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()); + } + + /// 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"); + } + + /// #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")); + } + + #[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()); + } } diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 0c7177d49..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,6 +498,9 @@ 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) { + if filepath.extension().is_some_and(|ext| ext == "py") { + 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,13 +519,6 @@ async fn prune_emptied_parents(pkg_path: &Path, filepath: &Path) { _ => return, } } - 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; - } - } for dir in chain.iter().rev() { if !remove_dir_if_empty(dir).await { return; @@ -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 { @@ -704,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; @@ -3077,4 +3050,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()); + } }