From 2d4d430aee35e69d1b55d2038bb0126818f1180c Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 12:42:27 -0400 Subject: [PATCH 1/3] WIP: Fix residual-reference keep reported as drift (#1184) Co-Authored-By: Claude Opus 5.5 (1M context) From 060e1c2ca4c20cc3a78e6db208bdb7cdc14c88f8 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 14:31:14 -0400 Subject: [PATCH 2/3] Stop calling a residual-reference keep drift When a vendored PyPI revert restored its recorded wiring but kept the wheel because another project file (a `pipenv requirements` export) still installs from it, `remove` and `rollback` reported the keep as "lockfile wiring drifted" and told the user to re-run `scan --mode vendored` to normalize, then remove. That remedy loops: the re-vendor re-wires Pipfile.lock and the next remove keeps the entry again. The keep now carries its cause. `vendor_artifact_kept`, the remove warning, skip reason and top-level error, rollback's vendoredKept reason and `vendor --revert` / reconcile skips say a project file still installs from the artifact, and the remedy is to point that file back at the registry release (or re-export it from the restored lock) and run the unwind again. A hosted takeover refused for the same reason names the file instead of a wiring edit. Drift keeps keep their existing wording. Fixes #1184 Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- .../socket-patch-cli/src/commands/remove.rs | 111 ++++++++++------ .../socket-patch-cli/src/commands/rollback.rs | 14 ++- .../src/commands/scan/hosted/takeover.rs | 44 +++++++ .../socket-patch-cli/src/commands/vendor.rs | 27 +++- .../src/commands/vendored_backend/mod.rs | 54 +++++++- .../tests/mode_migration_pypi.rs | 118 ++++++++++++++++++ crates/socket-patch-core/src/vendor/mod.rs | 36 ++++++ crates/socket-patch-core/src/vendor/pypi.rs | 13 +- 9 files changed, 364 insertions(+), 55 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bb4592f4b..412896093 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -1266,7 +1266,7 @@ Every `--json` invocation emits a single JSON object that follows the **unified | `vendor_state_retained` | `skipped` | remove `--skip-rollback`: vendor wiring + artifact deliberately left in place (the next `vendor` run reconciles the dropped entry). Also the top-level error code when `--skip-rollback` targets a vendored patch with no manifest record (every `scan`/`get --mode vendored` entry — and, v5.0, the ledger-only leftover of an earlier `remove --skip-rollback` of a manifest-tracked vendored patch). | | `hosted_state_retained` | (top-level error) | remove `--skip-rollback` targeting a hosted-only patch (no manifest entry): restoring the pin's upstream registry entry is the only possible removal, so the combination is refused (exit 1), mirroring the manifest-less vendored refusal above. | | `vendor_state_preserved` | `skipped` | remove `--preserve-state` (v5.0): lockfile unwired; artifact, ledger entry, and manifest entry all kept for a later re-apply. Rollback's counterpart is the `vendoredPreserved: []` envelope array. | -| `vendor_revert_kept` | `skipped` + top-level error | remove (v5.0): the vendored revert drift-kept (`kept_artifact`), so the ledger entry AND the manifest entry were both kept. ANY drift-keep makes the run a `partialFailure` (exit 1) — part of the requested removal did not happen; when EVERY matching entry drift-kept, the top-level error carries this code (`summary.removed` stays 0; the identifier DID match, so never `not_found`). Remedy: re-run `scan --mode vendored` to normalize, then remove. Rollback's counterpart is the `vendoredKept: []` envelope array (also exit 1). | +| `vendor_revert_kept` | `skipped` + top-level error | remove (v5.0): the vendored revert drift-kept (`kept_artifact`), so the ledger entry AND the manifest entry were both kept. ANY drift-keep makes the run a `partialFailure` (exit 1) — part of the requested removal did not happen; when EVERY matching entry drift-kept, the top-level error carries this code (`summary.removed` stays 0; the identifier DID match, so never `not_found`). Remedy: re-run `scan --mode vendored` to normalize, then remove. A PyPI revert that restored its recorded wiring but kept the wheel because another project file still installs from it (`vendor_revert_residual_reference`, e.g. a `pipenv requirements` / `uv export` / `poetry export` requirements file) is a keep too, but not a drift (#1184): its skip reason, the per-entry warning, the top-level message and rollback's `vendoredKept[].reason` say that a project file still installs from the vendored artifact, and the remedy is to point the file named by `vendor_revert_residual_reference` back at the registry release (or re-export it from the restored lock), then remove (or roll back) again. Its `vendor_artifact_kept` advisory says the same, and a hosted takeover refused for it (`redirect_vendored_revert_failed`) names the file instead of a wiring edit. Rollback's counterpart is the `vendoredKept: []` envelope array (also exit 1). | | `hosted_reverted` | `removed` | remove (v5.0): a hosted lockfile pin was restored to its upstream registry entry as part of removing the patch (`verified` on dry-run). Beside a manifest entry it bypasses `summary.removed` like `vendor_reverted`. | | `hosted_revert_failed` | top-level error | remove (v5.0): a matched hosted pin could not be restored to its upstream registry entry (`--offline`, a registry that does not answer, `bun.lockb`, a lock shape the restore refuses — see "Hosted unwind coverage"), or writing the restored files failed; the message names the `git checkout -- ` remedy. The manifest was not modified, exit 1. Rollback's counterpart is a `hosted.failed[]` entry (also `partial_failure` exit 1). v4's `hosted_revert_unsupported` is no longer emitted (every ecosystem has a restore). | | `reinstall_required` | rollback `warnings[]` | rollback (v5.0): vendored/hosted wiring was unwound, but installed trees keep their patched bytes until the next package-manager install — the stale-install advisory. When the run also emits a Bun advisory (`vendor_bun_reinstall_required` / `redirect_bun_reinstall_required`) the detail and the human note add " (Bun: a plain `bun install` keeps them; run `bun install --force`)". | diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index 6e3591fad..2babaf22c 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -18,7 +18,9 @@ use super::rollback::{rollback_patches_inner, InnerSelection}; use crate::args::{apply_env_toggles, GlobalArgs}; use crate::commands::hosted_unwind::{run_hosted_leg, HostedLegOutcome}; use crate::commands::lock_cli::acquire_or_emit; -use crate::commands::vendored_backend::{RevertedEntry, VendorRevertStep, VendoredBackend}; +use crate::commands::vendored_backend::{ + KeepCause, RevertedEntry, VendorRevertStep, VendoredBackend, +}; use crate::json_envelope::{Command, Envelope, EnvelopeError, PatchAction, PatchEvent, Status}; use crate::ui::short_uuid; use crate::ui::{plural, sweep_failure}; @@ -863,11 +865,7 @@ pub async fn run(args: RemoveArgs) -> i32 { // this is the only way the removal can be empty): the remove did // not happen. NOT not_found; partialFailure keeps `summary.removed` // honest at 0. - let msg = format!( - "{}: every matching entry's vendored state drift-kept; nothing was \ - removed (re-run `scan --mode vendored` to normalize, then remove)", - args.identifier - ); + let msg = all_kept_message(&args.identifier, &vendor_leg.kept_causes); track_patch_remove_failed(&msg, &telemetry).await; if args.common.json { let mut env = Envelope::new(Command::Remove); @@ -1125,21 +1123,62 @@ pub async fn run(args: RemoveArgs) -> i32 { // above are gated, so name the outcome once here. if !args.common.json { eprintln!( - "Error: {} matching entr{} drift-kept (vendored state and manifest \ - record retained); re-run `scan --mode vendored` to normalize, then \ - remove again", - vendor_leg.kept.len(), - if vendor_leg.kept.len() == 1 { - "y was" - } else { - "ies were" - } + "Error: {}", + kept_error_line(&vendor_leg.kept_causes, "manifest record") ); } 1 } } +/// The top-level error when every matching vendored entry was kept: the +/// drift wording and its normalize remedy only for drift-keeps (#1184). +fn all_kept_message(identifier: &str, causes: &[KeepCause]) -> String { + if causes.iter().all(|c| *c == KeepCause::Drift) { + return format!( + "{identifier}: every matching entry's vendored state drift-kept; nothing was \ + removed (re-run `scan --mode vendored` to normalize, then remove)" + ); + } + format!( + "{identifier}: every matching entry's vendored state was kept; nothing was removed \ + ({})", + kept_remedies(causes) + ) +} + +/// The human error line for kept entries (`retained` names the record kept +/// beside the vendored state). +fn kept_error_line(causes: &[KeepCause], retained: &str) -> String { + let n = causes.len(); + let entries = if n == 1 { "y was" } else { "ies were" }; + if causes.iter().all(|c| *c == KeepCause::Drift) { + return format!( + "{n} matching entr{entries} drift-kept (vendored state and {retained} \ + retained); re-run `scan --mode vendored` to normalize, then remove again" + ); + } + format!( + "{n} matching entr{entries} kept (vendored state and {retained} retained); {}", + kept_remedies(causes) + ) +} + +/// One remedy per distinct keep cause. +fn kept_remedies(causes: &[KeepCause]) -> String { + let mut out: Vec = Vec::new(); + for cause in [KeepCause::Reference, KeepCause::Drift] { + if causes.contains(&cause) { + let remedy = cause.remedy("remove again"); + out.push(match cause { + KeepCause::Reference => remedy, + KeepCause::Drift => format!("for a drift-kept entry, {remedy}"), + }); + } + } + out.join("; ") +} + /// The vendored leg's envelope material, collected by /// [`revert_vendored_matches`]. #[derive(Default)] @@ -1150,9 +1189,11 @@ struct RemoveVendorLeg { /// Backend warnings, drift-keeps and preserved entries — `Skipped` /// events. skipped: Vec, - /// Ledger keys whose revert drift-kept: entry, artifact and any + /// Ledger keys whose revert kept the artifact: entry, artifact and any /// manifest record stay. kept: Vec, + /// Why each [`Self::kept`] entry was kept (same order). + kept_causes: Vec, /// Entries actually reverted and dropped from the ledger (wet runs). reverted_count: usize, /// Entries unwired with their artifact and ledger entry kept @@ -1223,29 +1264,29 @@ async fn revert_vendored_matches( ); return Err(1); } - VendorRevertStep::Kept => { - // Drift-keep: the lock changed under us and the backend - // left both the wiring and the artifact alone. Per the - // RevertOutcome contract the ledger entry stays — and so - // must any manifest entry, or `vendor`'s reconcile would - // re-revert an entry whose backing record is gone. + VendorRevertStep::Kept(cause) => { + // The backend kept the artifact (a drift-keep, or a file + // that still installs from it). Per the RevertOutcome + // contract the ledger entry stays — and so must any + // manifest entry, or `vendor`'s reconcile would re-revert + // an entry whose backing record is gone. + let reason = cause.reason(); let (note, detail) = if manifest_backed { ( "; its manifest entry was kept too", - "lockfile wiring drifted; vendored state and manifest entry kept", + format!("{reason}; vendored state and manifest entry kept"), ) } else { ( "", - "lockfile wiring drifted; vendored state and ledger entry kept", + format!("{reason}; vendored state and ledger entry kept"), ) }; if loud { - eprintln!( - "Warning: Kept vendored state for {key}: lockfile wiring drifted{note}" - ); + eprintln!("Warning: Kept vendored state for {key}: {reason}{note}"); } leg.kept.push(key.clone()); + leg.kept_causes.push(cause); leg.skipped.push( PatchEvent::new(PatchAction::Skipped, key.clone()) .with_reason("vendor_revert_kept", detail), @@ -1571,11 +1612,7 @@ async fn remove_ledger_only( // top-level error names the outcome — nothing was removed. env.mark_partial_failure(); if leg.kept.len() == keys.len() { - let msg = format!( - "{}: every matching entry's vendored state drift-kept; nothing was \ - removed (re-run `scan --mode vendored` to normalize, then remove)", - args.identifier - ); + let msg = all_kept_message(&args.identifier, &leg.kept_causes); track_patch_remove_failed(&msg, telemetry).await; env.error = Some(EnvelopeError::new("vendor_revert_kept", msg)); } @@ -1593,14 +1630,8 @@ async fn remove_ledger_only( // are gated, so name the outcome once here. if !args.common.json { eprintln!( - "Error: {} matching entr{} drift-kept (vendored state and ledger record \ - retained); re-run `scan --mode vendored` to normalize, then remove again", - leg.kept.len(), - if leg.kept.len() == 1 { - "y was" - } else { - "ies were" - } + "Error: {}", + kept_error_line(&leg.kept_causes, "ledger record") ); } 1 diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 02bcdf968..bfcf7340f 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -27,7 +27,9 @@ use std::time::Duration; use crate::args::{apply_env_toggles, is_local_go, parse_bool_flag, GlobalArgs}; use crate::commands::hosted_unwind::run_hosted_leg; use crate::commands::lock_cli::acquire_or_emit; -use crate::commands::vendored_backend::{RevertedEntry, VendorRevertStep, VendoredBackend}; +use crate::commands::vendored_backend::{ + KeepCause, RevertedEntry, VendorRevertStep, VendoredBackend, +}; use crate::ecosystem_dispatch::{ distinct_npm_copies, find_all_packages_for_rollback, partition_purls, JvmScope, }; @@ -794,10 +796,18 @@ async fn run_vendored_leg( } out.failed.push((key, why)); } - VendorRevertStep::Kept => out.kept.push(( + VendorRevertStep::Kept(KeepCause::Drift) => out.kept.push(( key, "lockfile wiring drifted; vendored state left untouched".to_string(), )), + VendorRevertStep::Kept(cause @ KeepCause::Reference) => out.kept.push(( + key, + format!( + "{}; vendored state kept — {}", + cause.reason(), + cause.remedy("roll back again") + ), + )), VendorRevertStep::WouldRevert if preserve => { if loud { println!("Would unwire vendoring for {key} (artifact preserved)"); diff --git a/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs b/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs index 7a9ad136a..44765da21 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs @@ -253,6 +253,11 @@ impl Takeover { outcome.error.as_deref().unwrap_or("unknown error") ), })) + } else if let Some(residual) = residual_only_keep(&outcome) { + // Nothing drifted: another project file (an exported + // requirements file) still installs from the vendored + // wheel (#1184). Name it, not a drift. + Some(residual_takeover_warning(&purl, &residual)) } else if revert_keeps_wiring(&outcome) { // A wiring record drifted and was left in place, so the // project may still resolve through the vendored artifact @@ -552,6 +557,45 @@ fn revert_keeps_wiring(outcome: &RevertOutcome) -> bool { .any(|w| w.code == "vendor_revert_residual_reference") } +/// The `vendor_revert_residual_reference` detail of a revert whose ONLY +/// keep signal is a file that still references the artifact. +fn residual_only_keep(outcome: &RevertOutcome) -> Option { + if outcome.drift_skipped() { + return None; + } + outcome + .warnings + .iter() + .find(|w| w.code == socket_patch_core::vendor::RESIDUAL_REFERENCE_CODE) + .map(|w| { + // `kept : still resolves through it, and deleting …`: + // only the clause naming the file; the revert's own remedy is + // not this command's. + let detail = w.detail.as_str(); + detail + .split_once(": ") + .map_or(detail, |(_, rest)| rest) + .split(", and deleting") + .next() + .unwrap_or(detail) + .to_string() + }) +} + +/// The refusal for a takeover whose vendored wheel another project file +/// still installs from. +fn residual_takeover_warning(purl: &str, residual: &str) -> serde_json::Value { + serde_json::json!({ + "code": "redirect_vendored_revert_failed", + "detail": format!( + "{purl} is vendored and another project file still installs from its \ + vendored wheel ({residual}), so it is left in place; NOT switched to hosted — \ + point that file back at the registry release (re-export it once the hosted \ + scan has rewired the lock), then re-run `scan --mode hosted`" + ), + }) +} + /// The refusal for a takeover whose vendored wiring drifted since vendoring. fn drifted_takeover_warning(purl: &str) -> serde_json::Value { serde_json::json!({ diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index 29bcad9cd..bcd6bb205 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -51,7 +51,7 @@ use crate::commands::apply::{representative_file, result_to_event, variant_match use crate::commands::bun_preflight::bun_vendor_preflight_pairs; use crate::commands::lock_cli::acquire_or_emit; use crate::commands::vendored_backend::{ - ApplyRequest, RevertedEntry, VendorRevertStep, VendoredBackend, + ApplyRequest, KeepCause, RevertedEntry, VendorRevertStep, VendoredBackend, }; use crate::commands::vex::{ generate_vex_from_manifest_path, generate_vex_without_manifest, ManifestlessVex, VexEmbedArgs, @@ -4105,13 +4105,24 @@ pub(crate) async fn reconcile_dropped( // Drift-skip keep: the backend left the drifted lock alone and // kept the artifacts, so the ledger entry must survive too — and // the genuine outcome is a COUNTED skip, not a removal. - VendorRevertStep::Kept => env.record( + VendorRevertStep::Kept(KeepCause::Drift) => env.record( PatchEvent::new(PatchAction::Skipped, purl.clone()).with_reason( "vendor_revert_kept", "patch no longer in manifest, but its lock entries drifted since \ vendoring; artifacts and ledger entry kept", ), ), + VendorRevertStep::Kept(cause @ KeepCause::Reference) => env.record( + PatchEvent::new(PatchAction::Skipped, purl.clone()).with_reason( + "vendor_revert_kept", + format!( + "patch no longer in manifest, but {}; artifacts and ledger entry \ + kept — {}", + cause.reason(), + cause.remedy("re-run `vendor`") + ), + ), + ), VendorRevertStep::WouldRevert | VendorRevertStep::Reverted => { if !common.json && !common.silent { println!("{}", format_reconciled(&purl, common.dry_run)); @@ -4208,13 +4219,23 @@ async fn run_revert(args: &VendorArgs, env: &mut Envelope) -> i32 { // the genuine outcome is a COUNTED skip, not a removal. // (`record_warning` above already surfaced the per-record // details as uncounted advisory events.) - VendorRevertStep::Kept => env.record( + VendorRevertStep::Kept(KeepCause::Drift) => env.record( PatchEvent::new(PatchAction::Skipped, purl.clone()).with_reason( "vendor_revert_kept", "lock entries drifted since vendoring; artifacts and ledger entry kept \ — undo the drift and re-run `vendor --revert` to finish", ), ), + VendorRevertStep::Kept(cause @ KeepCause::Reference) => env.record( + PatchEvent::new(PatchAction::Skipped, purl.clone()).with_reason( + "vendor_revert_kept", + format!( + "{}; artifacts and ledger entry kept — {}", + cause.reason(), + cause.remedy("re-run `vendor --revert` to finish") + ), + ), + ), VendorRevertStep::WouldRevert | VendorRevertStep::Reverted => { env.record(PatchEvent::new(PatchAction::Removed, purl.clone())); reverted_flavors.extend(flavor); diff --git a/crates/socket-patch-cli/src/commands/vendored_backend/mod.rs b/crates/socket-patch-cli/src/commands/vendored_backend/mod.rs index 2b0461f30..750533108 100644 --- a/crates/socket-patch-cli/src/commands/vendored_backend/mod.rs +++ b/crates/socket-patch-cli/src/commands/vendored_backend/mod.rs @@ -148,10 +148,10 @@ pub(crate) enum VendorRevertStep { Missing, /// The backend refused; nothing changed for this entry. Failed(String), - /// Drift-keep: the lock changed under us and the backend left both the - /// wiring and the artifact alone. Per `RevertOutcome`'s contract the - /// ledger entry — and any manifest record — must survive. - Kept, + /// The backend kept the artifact (see [`KeepCause`]). Per + /// `RevertOutcome`'s contract the ledger entry — and any manifest + /// record — must survive. + Kept(KeepCause), /// Dry run: the revert (or, with `keep_artifact`, the unwire) would /// succeed. Nothing changed. WouldRevert, @@ -168,6 +168,46 @@ pub(crate) enum VendorRevertStep { LedgerWriteFailed(String), } +/// Why a vendored revert kept the artifact and ledger entry. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum KeepCause { + /// Drift-keep: the lock changed under us and the backend left both + /// the wiring and the artifact alone. + Drift, + /// The recorded wiring was restored, but another project file (an + /// exported requirements file) still installs from the artifact + /// (`vendor_revert_residual_reference`, #1184). Nothing drifted: the + /// way out is to point that file back at the registry release. + Reference, +} + +impl KeepCause { + /// The per-entry reason, after "`` kept". + pub(crate) fn reason(self) -> &'static str { + match self { + KeepCause::Drift => "lockfile wiring drifted", + KeepCause::Reference => { + "a project file still installs from the vendored artifact (see \ + vendor_revert_residual_reference); the recorded wiring was restored" + } + } + } + + /// The remedy that finishes the unwind, for `then` (the command to + /// re-run). + pub(crate) fn remedy(self, then: &str) -> String { + match self { + KeepCause::Drift => { + format!("re-run `scan --mode vendored` to normalize, then {then}") + } + KeepCause::Reference => format!( + "point the file named by vendor_revert_residual_reference back at the \ + registry release (or re-export it from the restored lock), then {then}" + ), + } + } +} + pub(crate) struct VendorRevertResult { pub(crate) warnings: Vec, pub(crate) step: VendorRevertStep, @@ -190,7 +230,11 @@ pub(crate) async fn revert_vendor_entry( let step = if !outcome.success { VendorRevertStep::Failed(outcome.error.unwrap_or_else(|| "unknown error".into())) } else if outcome.kept_artifact { - VendorRevertStep::Kept + VendorRevertStep::Kept(if outcome.kept_for_residual_reference() { + KeepCause::Reference + } else { + KeepCause::Drift + }) } else if opts.dry_run { VendorRevertStep::WouldRevert } else if opts.keep_artifact { diff --git a/crates/socket-patch-cli/tests/mode_migration_pypi.rs b/crates/socket-patch-cli/tests/mode_migration_pypi.rs index 9296c6884..84de007b7 100644 --- a/crates/socket-patch-cli/tests/mode_migration_pypi.rs +++ b/crates/socket-patch-cli/tests/mode_migration_pypi.rs @@ -2002,3 +2002,121 @@ async fn pipenv_hosted_to_vendored_names_the_unpatched_requirements() { "the takeover names requirements.txt as an unpatched install source: {env:#}" ); } + +/// #1184: a vendored Pipenv project whose `pipenv requirements > +/// requirements.txt` export (made after vendoring) still installs from the +/// vendored wheel. Every unwind restores Pipfile.lock and keeps the wheel +/// and ledger entry while that export references it — correct — but must +/// say so: no "lockfile wiring drifted" wording, no "re-run `scan --mode +/// vendored` to normalize" remedy (that loops), and a remedy naming the +/// file to re-export. Once the export is pointed back at the registry, the +/// same unwind finishes. +#[tokio::test] +async fn pipenv_residual_export_keep_is_not_reported_as_drift() { + let wheel_dir = format!(".socket/vendor/pypi/{UUID}"); + let export = format!( + "-i https://pypi.org/simple\n./{wheel_dir}/{WHEEL} ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'\n" + ); + let no_drift = |label: &str, text: &str| { + for bad in ["drift", "normalize"] { + assert!( + !text.contains(bad), + "{label}: a residual-reference keep is not drift ({bad:?}):\n{text}" + ); + } + assert!( + text.contains("vendor_revert_residual_reference") || text.contains("requirements.txt"), + "{label}: the keep names the referencing file:\n{text}" + ); + assert!( + text.contains("re-export"), + "{label}: the remedy is to re-export the file:\n{text}" + ); + }; + for unwind in [ + vec!["remove", PURL, "--yes"], + vec!["rollback", "--yes"], + vec!["vendor", "--revert"], + ] { + let (_tmp, root) = project(); + let files = stage_pipenv(&root); + let pristine = std::fs::read_to_string(root.join("Pipfile.lock")).unwrap(); + vendor_project(&root, files); + std::fs::write(root.join("requirements.txt"), &export).unwrap(); + + let (code, env) = run_cli(&root, &unwind, &[]); + let label = format!("{unwind:?}"); + no_drift(&label, &env.to_string()); + if unwind[0] == "remove" { + assert_eq!(code, 1, "{label}: the removal is not finished: {env:#}"); + let (code, _stdout, stderr) = run_raw(&root, &unwind, &[]); + assert_eq!(code, 1, "{label} (human): {stderr}"); + no_drift(&format!("{label} (human)"), &stderr); + } + let lock = |text: &str| serde_json::from_str::(text).unwrap(); + assert_eq!( + lock(&std::fs::read_to_string(root.join("Pipfile.lock")).unwrap()), + lock(&pristine), + "{label}: Pipfile.lock is restored" + ); + assert!( + root.join(&wheel_dir).exists(), + "{label}: the wheel the export installs from is kept" + ); + + // The prescribed fix: re-export from the restored lock. + std::fs::write( + root.join("requirements.txt"), + "-i https://pypi.org/simple\nsix==1.16.0\n", + ) + .unwrap(); + let (code, env) = run_cli(&root, &unwind, &[]); + assert_eq!(code, 0, "{label} after the re-export: {env:#}"); + assert!( + !root.join(&wheel_dir).exists(), + "{label}: the wheel is reclaimed once nothing references it" + ); + } +} + +/// #1184, the hosted takeover lane: the same export keeps the vendored +/// wheel, so the takeover refuses (the package stays vendored and patched) +/// and must name the export, not "wiring edited since vendoring" with a +/// `vendor --revert` remedy that leaves the project unpatched. +#[tokio::test] +async fn pipenv_residual_export_takeover_refusal_names_the_export() { + let (_tmp, root) = project(); + let files = stage_pipenv(&root); + vendor_project(&root, files); + std::fs::write( + root.join("requirements.txt"), + format!("-i https://pypi.org/simple\n./.socket/vendor/pypi/{UUID}/{WHEEL}\n"), + ) + .unwrap(); + let vendored = std::fs::read_to_string(root.join("Pipfile.lock")).unwrap(); + let server = MockServer::start().await; + mount_hosted_api(&server, true).await; + let (code, env) = hosted_scan(&root, &server); + assert_eq!(code, 0, "{env:#}"); + assert_eq!(env["redirect"]["redirected"], 0, "{env:#}"); + let detail = env["redirect"]["warnings"] + .as_array() + .unwrap() + .iter() + .find(|w| w["code"] == "redirect_vendored_revert_failed") + .and_then(|w| w["detail"].as_str()) + .unwrap_or_else(|| panic!("the takeover is refused: {env:#}")) + .to_string(); + assert!( + detail.contains("requirements.txt") + && detail.contains("re-export") + && !detail.contains("edited since vendoring") + && !detail.contains("vendor --revert"), + "{detail}" + ); + assert_eq!( + std::fs::read_to_string(root.join("Pipfile.lock")).unwrap(), + vendored, + "the package stays vendored" + ); +} diff --git a/crates/socket-patch-core/src/vendor/mod.rs b/crates/socket-patch-core/src/vendor/mod.rs index d37d81f64..ede31411d 100644 --- a/crates/socket-patch-core/src/vendor/mod.rs +++ b/crates/socket-patch-core/src/vendor/mod.rs @@ -687,6 +687,12 @@ impl RevertOpts { /// time (the dependency was removed). See [`RevertOutcome::lock_entry_removed`]. pub const LOCK_ENTRY_REMOVED_CODE: &str = "vendor_lock_entry_removed"; +/// A PyPI revert restored the wiring it recorded, but another project file +/// (a `pipenv requirements` / `uv export` / `poetry export` requirements +/// file, a moved vendor line) still installs from the vendored wheel, so +/// the artifact and ledger entry are kept until nothing references them. +pub const RESIDUAL_REFERENCE_CODE: &str = "vendor_revert_residual_reference"; + /// The result of one backend `revert_*` call. #[derive(Debug)] pub struct RevertOutcome { @@ -762,6 +768,36 @@ impl RevertOutcome { .any(|w| w.code == LOCK_ENTRY_REMOVED_CODE) } + /// True when the artifact was kept ONLY because another project file + /// still references it ([`RESIDUAL_REFERENCE_CODE`]): the recorded + /// wiring was restored and nothing drifted, so the way out is to point + /// that file back at the registry release, not to undo a drift (#1184). + pub fn kept_for_residual_reference(&self) -> bool { + self.kept_artifact + && !self.drift_skipped() + && self + .warnings + .iter() + .any(|w| w.code == RESIDUAL_REFERENCE_CODE) + } + + /// [`Self::keep_artifact`] for a residual-reference keep: the wiring + /// was restored, but a project file the revert does not own still + /// installs from `uuid_dir_rel` (named by the + /// [`RESIDUAL_REFERENCE_CODE`] warning). + pub fn keep_artifact_for_reference(&mut self, uuid_dir_rel: &str) { + self.kept_artifact = true; + self.warnings.push(VendorWarning::new( + "vendor_artifact_kept", + format!( + "kept {uuid_dir_rel}: the recorded wiring was restored, but a project file \ + still installs from it (see the vendor_revert_residual_reference warning); \ + point that file back at the registry release (or re-export it from the \ + restored lock) and re-run the revert to finish cleaning up" + ), + )); + } + /// Mark the artifact dir as deliberately kept after a drift-skip and /// surface it honestly. Backends call this INSTEAD of removing the /// uuid dir when [`Self::drift_skipped`] is true: deleting it would be diff --git a/crates/socket-patch-core/src/vendor/pypi.rs b/crates/socket-patch-core/src/vendor/pypi.rs index 73822f574..4c4eb46b8 100644 --- a/crates/socket-patch-core/src/vendor/pypi.rs +++ b/crates/socket-patch-core/src/vendor/pypi.rs @@ -2274,7 +2274,8 @@ fn residual_reference_warning(uuid: &str, clause: &str) -> VendorWarning { format!( "kept .socket/vendor/pypi/{uuid}/: {clause}, and deleting the vendored wheel would \ make every install from it fail; point that file back at the registry release \ - (or re-export it from the restored lock) and re-run `vendor --revert`" + (or re-export it from the restored lock) and re-run the revert (`vendor \ + --revert`, `remove` or `rollback`)" ), ) } @@ -2395,14 +2396,18 @@ pub async fn revert_pypi_opts( || outcome .warnings .iter() - .any(|w| w.code == "vendor_revert_residual_reference") + .any(|w| w.code == super::RESIDUAL_REFERENCE_CODE) { // Display-only path: with a non-canonical uuid nothing below would // have been deleted anyway, but the drift-keep must still be // surfaced so the ledger entry survives. let uuid_dir_rel = vendor_uuid_dir_rel("pypi", &entry.uuid) .unwrap_or_else(|| format!(".socket/vendor/pypi/{:?}", entry.uuid)); - outcome.keep_artifact(&uuid_dir_rel); + if outcome.drift_skipped() { + outcome.keep_artifact(&uuid_dir_rel); + } else { + outcome.keep_artifact_for_reference(&uuid_dir_rel); + } return outcome; } // `--preserve-state` (`keep_artifact`): the wiring restore above already @@ -2426,7 +2431,7 @@ pub async fn revert_pypi_opts( .push(residual_reference_warning(&entry.uuid, &clause)); let uuid_dir_rel = vendor_uuid_dir_rel("pypi", &entry.uuid) .unwrap_or_else(|| format!(".socket/vendor/pypi/{:?}", entry.uuid)); - outcome.keep_artifact(&uuid_dir_rel); + outcome.keep_artifact_for_reference(&uuid_dir_rel); return outcome; } } From 7407bf67d7be647813cd611fc1b7ce7d14954b21 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Sat, 10 Oct 2026 10:18:51 -0400 Subject: [PATCH 3/3] Give the residual takeover refusal a remedy order that converges The refusal leaves the lock vendored, so re-exporting the requirements file from it would name the wheel again and the next hosted scan would refuse again. Tell the user to pin the file to == by hand, re-run the hosted scan, and only then re-export; the takeover test now follows that remedy through to a completed takeover. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/commands/scan/hosted/takeover.rs | 18 ++++++++++++++++-- .../tests/mode_migration_pypi.rs | 16 ++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs b/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs index 44765da21..a5c975a17 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted/takeover.rs @@ -584,14 +584,28 @@ fn residual_only_keep(outcome: &RevertOutcome) -> Option { /// The refusal for a takeover whose vendored wheel another project file /// still installs from. +/// +/// The order matters: the refusal leaves the lock vendored, so re-exporting +/// from it now would name the wheel again. The file is pinned by hand +/// first; only after the hosted scan has rewired the lock is a re-export +/// safe. fn residual_takeover_warning(purl: &str, residual: &str) -> serde_json::Value { + let pin = purl + .strip_prefix("pkg:pypi/") + .map(|rest| rest.split(['?', '#']).next().unwrap_or(rest)) + .and_then(|rest| rest.split_once('@')) + .map_or_else( + || "the registry release".to_string(), + |(name, version)| format!("`{name}=={version}`"), + ); serde_json::json!({ "code": "redirect_vendored_revert_failed", "detail": format!( "{purl} is vendored and another project file still installs from its \ vendored wheel ({residual}), so it is left in place; NOT switched to hosted — \ - point that file back at the registry release (re-export it once the hosted \ - scan has rewired the lock), then re-run `scan --mode hosted`" + first replace the wheel path in that file with {pin} by hand (the lock is \ + still vendored, so a re-export now would name the wheel again), re-run \ + `scan --mode hosted`, and only then re-export the file from the rewired lock" ), }) } diff --git a/crates/socket-patch-cli/tests/mode_migration_pypi.rs b/crates/socket-patch-cli/tests/mode_migration_pypi.rs index 1ec709eb0..f65b529fe 100644 --- a/crates/socket-patch-cli/tests/mode_migration_pypi.rs +++ b/crates/socket-patch-cli/tests/mode_migration_pypi.rs @@ -2401,6 +2401,7 @@ async fn pipenv_residual_export_takeover_refusal_names_the_export() { .to_string(); assert!( detail.contains("requirements.txt") + && detail.contains("`six==1.16.0`") && detail.contains("re-export") && !detail.contains("edited since vendoring") && !detail.contains("vendor --revert"), @@ -2411,4 +2412,19 @@ async fn pipenv_residual_export_takeover_refusal_names_the_export() { vendored, "the package stays vendored" ); + + // The prescribed order converges: pin the file by hand, then the + // hosted scan takes the package over. + std::fs::write( + root.join("requirements.txt"), + "-i https://pypi.org/simple\nsix==1.16.0\n", + ) + .unwrap(); + let (code, env) = hosted_scan(&root, &server); + assert_eq!(code, 0, "{env:#}"); + assert_eq!(env["redirect"]["redirected"], 1, "{env:#}"); + assert!( + !root.join(format!(".socket/vendor/pypi/{UUID}")).exists(), + "the takeover reclaims the vendored wheel: {env:#}" + ); }