diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index 3965a9ee2..f51222fa2 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -1295,9 +1295,15 @@ async fn revert_vendored_matches( let mut leg = RemoveVendorLeg::default(); // Stops at the first hard failure: remove aborts there, leaving the // remaining matches (and the manifest) untouched. - let reverted = VendoredBackend::new(&args.common, None) - .revert(keys, state, opts, true) - .await; + let backend = VendoredBackend::new(&args.common, None); + let reverted = backend.revert(keys, state, opts, true).await; + for (key, w) in backend.go_undo_warnings(&reverted).await { + if loud { + eprintln!("Warning: {}", w.detail); + } + leg.skipped + .push(PatchEvent::new(PatchAction::Skipped, key).with_reason(w.code, w.detail)); + } for RevertedEntry { key, warnings, diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 6f98e9957..9a0c266d9 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -989,9 +989,14 @@ async fn run_vendored_leg( keep_artifact: preserve, }; let loud = !common.json && !common.silent; - let reverted = VendoredBackend::new(common, None) - .revert(keys, state, opts, false) - .await; + let backend = VendoredBackend::new(common, None); + let reverted = backend.revert(keys, state, opts, false).await; + for (_, w) in backend.go_undo_warnings(&reverted).await { + if loud { + eprintln!("Warning: {}", w.detail); + } + out.warnings.push((w.code.to_string(), w.detail)); + } for RevertedEntry { key, warnings, diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index bd85b661f..0bcb450f5 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -1241,7 +1241,8 @@ async fn run_check(args: &VendorArgs) -> i32 { } else { None }; - let go_sync_issues = tokio::sync::OnceCell::new(); + let go_sync_issues = + socket_patch_core::vendor::go_consumer_sync::audit(root, &HashMap::new()).await; for (key, entry) in entries { let record = entry.record.as_ref().or_else(|| manifest.patches.get(key)); let mut failure = match record { @@ -1280,11 +1281,6 @@ async fn run_check(args: &VendorArgs) -> i32 { if failure.is_none() && entry.ecosystem == "golang" { if let Some((module, _)) = socket_patch_core::utils::purl::parse_golang_purl(key) { let issues: Vec = go_sync_issues - .get_or_init(|| async { - socket_patch_core::vendor::go_consumer_sync::audit(root, &HashMap::new()) - .await - }) - .await .iter() .filter(|issue| issue.module() == module) .map(ToString::to_string) @@ -1362,6 +1358,46 @@ async fn run_check(args: &VendorArgs) -> i32 { .with_details(serde_json::json!({ "ecosystem": eco, "path": rel })), ); } + // A committed vendor/modules.txt still recording a socket `replace` + // that go.mod no longer has breaks every default build until it is + // regenerated (#1777). The ledger entry it came from is gone after a + // revert, so the per-entry check above cannot see it. + let ledger_modules: HashSet = state + .entries + .iter() + .filter(|(_, entry)| entry.ecosystem == "golang") + .filter_map(|(key, _)| socket_patch_core::utils::purl::parse_golang_purl(key)) + .map(|(module, _)| module.into_owned()) + .collect(); + let stale_records: Vec<(String, String)> = go_sync_issues + .iter() + .filter(|issue| { + matches!( + issue, + socket_patch_core::vendor::go_consumer_sync::GoSyncIssue::VendorModulesTxt { + stale: true, + .. + } + ) && !ledger_modules.contains(issue.module()) + }) + .map(|issue| (issue.module().to_string(), issue.to_string())) + .collect(); + for (module, detail) in stale_records { + if !args.common.json { + eprintln!( + "{}: {detail}", + socket_patch_core::vendor::go_consumer_sync::VENDOR_MODULES_TXT_CODE + ); + } + env.record( + PatchEvent::artifact(PatchAction::Failed) + .with_error( + socket_patch_core::vendor::go_consumer_sync::VENDOR_MODULES_TXT_CODE, + detail, + ) + .with_details(serde_json::json!({ "ecosystem": "golang", "module": module })), + ); + } if args.common.json { println!("{}", env.to_pretty_json()); } @@ -4436,7 +4472,8 @@ async fn run_revert(args: &VendorArgs, env: &mut Envelope) -> i32 { // (rollback's vendored leg, both of remove's paths, the manifest // reconcile): dispatch → drift-keep → per-entry ledger save. Only the // event vocabulary and the human lines are this command's. - let reverted = VendoredBackend::new(common, None) + let backend = VendoredBackend::new(common, None); + let reverted = backend .revert( &recorded, &mut state, @@ -4444,6 +4481,9 @@ async fn run_revert(args: &VendorArgs, env: &mut Envelope) -> i32 { false, ) .await; + for (purl, w) in backend.go_undo_warnings(&reverted).await { + record_warning(env, &purl, &w, common); + } for RevertedEntry { key: purl, 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 860e90809..a00d1e93c 100644 --- a/crates/socket-patch-cli/src/commands/vendored_backend/mod.rs +++ b/crates/socket-patch-cli/src/commands/vendored_backend/mod.rs @@ -199,6 +199,49 @@ impl<'a> VendoredBackend<'a> { } out } + + /// Dropping a vendored Go `replace` leaves a committed + /// vendor/modules.txt still recording it, which breaks every default + /// build until `go mod vendor` runs (#343, #1777). The undo commands + /// call this after [`Self::revert`]: it audits the Go consumer state + /// once and returns the issues of the Go modules `reverted` unwired, + /// each keyed by the ledger key of its entry. Not part of `revert` + /// itself, whose forward-run caller (the manifest reconcile) audits + /// once at the end of the run instead. + pub(crate) async fn go_undo_warnings( + &self, + reverted: &[RevertedEntry], + ) -> Vec<(String, VendorWarning)> { + let unwired: Vec<(&str, String)> = reverted + .iter() + .filter(|e| { + matches!( + e.step, + VendorRevertStep::Reverted + | VendorRevertStep::Preserved + | VendorRevertStep::LedgerWriteFailed(_) + ) + }) + .filter_map(|e| { + socket_patch_core::utils::purl::parse_golang_purl(&e.key) + .map(|(module, _)| (e.key.as_str(), module.into_owned())) + }) + .collect(); + if unwired.is_empty() { + return Vec::new(); + } + socket_patch_core::vendor::go_consumer_sync::audit(&self.common.cwd, &HashMap::new()) + .await + .into_iter() + .filter_map(|issue| { + let (key, _) = unwired.iter().find(|(_, m)| m == issue.module())?; + Some(( + key.to_string(), + VendorWarning::new(issue.code(), issue.to_string()), + )) + }) + .collect() + } } /// Name a patched `-.gem` left in Bundler's cache dir once diff --git a/crates/socket-patch-cli/tests/e2e_vendor_golang_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_golang_build.rs index 852cfc0ef..bd772e205 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_golang_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_golang_build.rs @@ -1035,3 +1035,176 @@ fn go_vendor_reports_applied_event() { chmod_writable(tmp.path()); } + +// ── #1777: vendored undo vs a committed vendor/ ─────────────────────── + +/// #1777: in a project with a committed `vendor/`, a vendored patch plus +/// the `go mod vendor` its forward warning asks for leaves +/// `vendor/modules.txt` recording the socket `replace`. Every vendored undo +/// (`vendor --revert`, `rollback`, `remove `) drops that `replace`, so +/// the next default build fails with "inconsistent vendoring" until +/// `go mod vendor` runs again. Each undo must name that regeneration (as +/// agent-mode rollback does, #343), and `apply --check` must report the +/// stale modules.txt as drift until it is run. +fn vendored_undo_needs_go_mod_vendor(label: &str, undo: &[&str], workspace: bool) { + if !golang_e2e_matrix::toolchain_ready(&format!("e2e_vendor_golang_build({label})")) { + return; + } + let tmp = tempfile::tempdir().unwrap(); + let (consumer, modcache, proxy) = stage(tmp.path()); + let mc = modcache.to_str().unwrap(); + let mut goenv = go_env(mc, &proxy); + // In a workspace, `go work vendor` owns vendor/modules.txt. + let regen_cmd: &[&str] = if workspace { + std::fs::write(consumer.join("go.work"), "go 1.21\n\nuse .\n").unwrap(); + &["work", "vendor"] + } else { + &["mod", "vendor"] + }; + if workspace { + // `-mod=mod` is not allowed in workspace mode. + goenv.retain(|(k, _)| *k != "GOFLAGS"); + goenv.push(("GOFLAGS", "")); + } + let vendored = go(&consumer, regen_cmd, &goenv); + assert!( + vendored.status.success(), + "{regen_cmd:?}: {}", + String::from_utf8_lossy(&vendored.stderr) + ); + // Default flags from here on: go builds from vendor/. + goenv.retain(|(k, _)| *k != "GOFLAGS"); + goenv.push(("GOFLAGS", "")); + + write_patch(&consumer); + let (code, stdout, stderr) = run_socket( + &consumer, + &[ + "vendor", + "--json", + "--offline", + "--cwd", + consumer.to_str().unwrap(), + ], + &modcache, + ); + assert_eq!( + code, 0, + "vendor failed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let revendor = go(&consumer, regen_cmd, &goenv); + assert!( + revendor.status.success(), + "{}", + String::from_utf8_lossy(&revendor.stderr) + ); + let run = go(&consumer, &["run", "."], &goenv); + assert!( + String::from_utf8_lossy(&run.stdout).contains("OUT: PATCHED"), + "{}{}", + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + + { + let cs = consumer.to_str().unwrap(); + let mut args = undo.to_vec(); + args.extend(["--cwd", cs]); + let (code, so, se) = run_socket(&consumer, &args, &modcache); + assert_eq!(code, 0, "[{label}] failed.\n{so}\n{se}"); + let renv = parse_json_envelope(&so); + assert!( + !std::fs::read_to_string(consumer.join("go.mod")) + .unwrap() + .contains("replace "), + "[{label}] the socket replace is gone" + ); + let broken = go(&consumer, &["build", "./..."], &goenv); + assert!( + !broken.status.success() + && String::from_utf8_lossy(&broken.stderr).contains("inconsistent vendoring"), + "[{label}] the fixture reproduces the broken build: {}", + String::from_utf8_lossy(&broken.stderr) + ); + assert!( + common::envelope::all_codes(&renv) + .contains(&"go_vendor_modules_txt_out_of_sync".to_string()), + "[{label}] must name the vendor/modules.txt regeneration:\n{renv}" + ); + let (code, so, se) = run_socket( + &consumer, + &["apply", "--check", "--ecosystems", "golang", "--cwd", cs], + &modcache, + ); + assert_eq!( + code, 1, + "[{label}] stale vendor/modules.txt is drift:\n{so}\n{se}" + ); + let named = format!("go {}", regen_cmd.join(" ")); + assert!(se.contains(&named), "[{label}] must name `{named}`: {se}"); + let (code, so, se) = run_socket(&consumer, &["vendor", "--check", "--cwd", cs], &modcache); + assert_eq!( + code, 1, + "[{label}] vendor --check: stale vendor/modules.txt is drift:\n{so}\n{se}" + ); + assert!(se.contains(&named), "[{label}] must name `{named}`: {se}"); + + // The step the warning names fixes the build. + let regen = go(&consumer, regen_cmd, &goenv); + assert!( + regen.status.success(), + "[{label}] {}", + String::from_utf8_lossy(®en.stderr) + ); + let run = go(&consumer, &["run", "."], &goenv); + assert!( + String::from_utf8_lossy(&run.stdout).contains("OUT: PRISTINE"), + "[{label}] {}{}", + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + // (`vendor --revert` keeps the manifest record, which `--check` + // still reports as unledgered; only the modules.txt drift is gone.) + let (_code, so, se) = run_socket(&consumer, &["vendor", "--check", "--cwd", cs], &modcache); + assert!( + !se.contains("go_vendor_modules_txt_out_of_sync") && !so.contains("modules.txt"), + "[{label}] in sync after {named}:\n{so}\n{se}" + ); + } + + chmod_writable(tmp.path()); +} + +const REVERT: &[&str] = &["vendor", "--revert", "--json"]; +const ROLLBACK: &[&str] = &["rollback", "--offline", "--json"]; +const REMOVE: &[&str] = &["remove", UPURL, "--offline", "--json"]; + +#[test] +fn vendor_revert_needs_go_mod_vendor_with_committed_vendor_dir() { + vendored_undo_needs_go_mod_vendor("vendor --revert", REVERT, false); +} + +#[test] +fn vendored_rollback_needs_go_mod_vendor_with_committed_vendor_dir() { + vendored_undo_needs_go_mod_vendor("rollback", ROLLBACK, false); +} + +#[test] +fn vendored_remove_needs_go_mod_vendor_with_committed_vendor_dir() { + vendored_undo_needs_go_mod_vendor("remove", REMOVE, false); +} + +#[test] +fn vendor_revert_needs_go_work_vendor_in_workspace() { + vendored_undo_needs_go_mod_vendor("go.work vendor --revert", REVERT, true); +} + +#[test] +fn vendored_rollback_needs_go_work_vendor_in_workspace() { + vendored_undo_needs_go_mod_vendor("go.work rollback", ROLLBACK, true); +} + +#[test] +fn vendored_remove_needs_go_work_vendor_in_workspace() { + vendored_undo_needs_go_mod_vendor("go.work remove", REMOVE, true); +} diff --git a/docs/ecosystems.md b/docs/ecosystems.md index 62b0b8556..18063b65b 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -1475,8 +1475,9 @@ for it until `rollback -g`. Two files the go command derives from `go.mod` can disagree with a new or removed socket `replace`, and go then refuses to build. socket-patch does not regenerate -them (that needs the go toolchain and the whole module graph); `apply`, `vendor`, -`rollback` and `scan --mode hosted` warn with the command that does, and +them (that needs the go toolchain and the whole module graph); `apply`, `vendor` +(including `vendor --revert`), `rollback`, `remove` and `scan --mode hosted` warn +with the command that does, and `apply --check` / `vendor --check` report the disagreement as drift until it is run: | Warning code | When | Fix |