diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index a7a29236..4605d475 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -67,7 +67,13 @@ fn parse_purl_simple(purl: &str) -> Option<(String, String, String)> { let (typ, after) = rest.split_once('/')?; let (coord, version) = after.rsplit_once('@')?; let name = socket_patch_core::utils::purl::percent_decode_purl_component(coord).into_owned(); - Some((typ.to_string(), name, version.to_string())) + // The API serves canonical percent-encoded purls, so the version needs + // decoding just like the coordinate — npm build metadata arrives as + // `1.2.3%2Bbuild` while lockfiles store `1.2.3+build`; an undecoded + // version would silently match no lock entry. + let version = + socket_patch_core::utils::purl::percent_decode_purl_component(version).into_owned(); + Some((typ.to_string(), name, version)) } /// The hosted-mode JSON error envelope, for bail-outs that return before the @@ -556,7 +562,8 @@ pub(super) async fn run_redirect( // re-run produces no new edits (the lockfile already points at the // hosted patch), and clobbering the file would lose the original // pre-redirect values a future revert needs. New edits APPEND (revert - // walks them in reverse); records are keyed by PURL, newest wins. + // walks them in reverse), skipping byte-identical re-plans from a + // retried partial failure; records are keyed by PURL, newest wins. // // Persisted BEFORE the project files, and atomically (stage + fsync + // rename, like the sibling vendor ledger): a crash between the two @@ -573,12 +580,15 @@ pub(super) async fn run_redirect( ledger.mode = "hosted".to_string(); // The bun.lockb→bun.lock migration removal precedes the rewrite // edits so `--revert` unwinds it last (after restoring bun.lock). - ledger.edits.extend(migration_edits.iter().cloned()); - ledger.edits.extend(rewrite.edits.iter().cloned()); + for edit in migration_edits.iter().chain(rewrite.edits.iter()) { + if !ledger.edits.contains(edit) { + ledger.edits.push(edit.clone()); + } + } ledger.records.extend(records.clone()); // The ledger is the only revert path and the VEX record store — - // a swallowed write failure would leave the rewritten lockfiles - // unrevertable while reporting success. + // a swallowed write failure would let the lockfile writes below + // proceed with no revert data persisted while reporting success. if let Err(e) = socket_patch_core::patch::redirect::save_redirect_state(&args.common.cwd, &ledger) .await @@ -596,7 +606,15 @@ pub(super) async fn run_redirect( if let Some(parent) = path.parent() { let _ = std::fs::create_dir_all(parent); } - if let Err(e) = std::fs::write(&path, content) { + // Atomic stage+rename, mode-preserving (the vendored backend's + // writer): a bare `fs::write` truncates first, so a crash + // mid-write could leave a torn lockfile behind. + if let Err(e) = socket_patch_core::utils::fs::atomic_write_bytes_preserving_mode( + &path, + content.as_bytes(), + ) + .await + { let message = format!("failed to write {rel}: {e}"); eprintln!("{message}"); if args.common.json { @@ -767,9 +785,42 @@ pub(super) async fn run_redirect( #[cfg(test)] mod tests { - use super::{build_redirect_json_envelope, REDIRECT_CANDIDATE_FILES}; + use super::{build_redirect_json_envelope, parse_purl_simple, REDIRECT_CANDIDATE_FILES}; use socket_patch_core::constants::npm_family; + #[test] + fn parse_purl_simple_percent_decodes_name_and_version() { + // The API serves canonical percent-encoded purls: npm build metadata + // `1.2.3+build` arrives as `1.2.3%2Bbuild`. Lock entries store the + // decoded form, so an undecoded version silently matches nothing. + assert_eq!( + parse_purl_simple("pkg:npm/foo@1.2.3%2Bbuild"), + Some(( + "npm".to_string(), + "foo".to_string(), + "1.2.3+build".to_string() + )) + ); + // The coordinate keeps decoding too (scoped npm name). + assert_eq!( + parse_purl_simple("pkg:npm/%40scope/name@1.0.0"), + Some(( + "npm".to_string(), + "@scope/name".to_string(), + "1.0.0".to_string() + )) + ); + // Plain versions pass through unchanged. + assert_eq!( + parse_purl_simple("pkg:npm/left-pad@1.3.0"), + Some(( + "npm".to_string(), + "left-pad".to_string(), + "1.3.0".to_string() + )) + ); + } + /// The classic scan object `run` builds for the `--json` path with ≥1 /// discovered package (scannedPackages/totalPatches/… + the `packages` /// enumeration). Mirrors the `serde_json::json!` in `scan::run`. diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index 52c2d61c..07ea8126 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -1290,6 +1290,94 @@ async fn no_redirectable_patch_leaves_bun_lockb_alone() { ); } +/// The corrupt-ledger refusal must fire BEFORE the bun.lockb auto-migration, +/// not after it. The migration deletes the binary lock and writes a text one, +/// so running it ahead of the refusal would convert the project's lockfile +/// format and then exit 1 without recording the migration or redirecting +/// anything — the "byte-untouched on refusal" promise broken by the one write +/// that precedes every rewriter. A redirectable npm override is granted here +/// (the migration's gate) and a fake `bun` that WOULD migrate sits on PATH, so +/// the surviving bun.lockb proves the ordering rather than a skipped gate. +#[cfg(unix)] +#[tokio::test] +#[serial] +async fn corrupt_ledger_refuses_before_the_bun_lockb_migration() { + let server = MockServer::start().await; + mock_discovery(&server).await; + mock_reference(&server).await; + mock_view(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write( + tmp.path().join("package.json"), + format!( + r#"{{ "name": "consumer", "version": "0.0.0", "dependencies": {{ "{NAME}": "^{VERSION}" }} }}"# + ), + ) + .unwrap(); + let pkg = tmp.path().join("node_modules").join(NAME); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + format!(r#"{{ "name": "{NAME}", "version": "{VERSION}" }}"#), + ) + .unwrap(); + std::fs::write(tmp.path().join("bun.lockb"), b"BUN-BINARY-PLACEHOLDER").unwrap(); + + // A torn ledger: parseable as neither the vendor nor the redirect shape. + let ledger_path = tmp.path().join(".socket/vendor/redirect-state.json"); + std::fs::create_dir_all(ledger_path.parent().unwrap()).unwrap(); + let corrupt_bytes = b"{\"mode\":\"hosted\",\"edits\":[{\"path\":\"bun.lo"; + std::fs::write(&ledger_path, corrupt_bytes).unwrap(); + + let bin_dir = tmp.path().join("fakebin"); + std::fs::create_dir_all(&bin_dir).unwrap(); + let shim = bin_dir.join("bun"); + std::fs::write( + &shim, + "#!/bin/sh\n\ + echo '{ \"lockfileVersion\": 1, \"packages\": {} }' > bun.lock\n\ + rm -f bun.lockb\n\ + exit 0\n", + ) + .unwrap(); + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&shim, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + let orig_path = std::env::var("PATH").unwrap_or_default(); + // SAFETY: single-threaded #[serial] test; PATH restored below. + unsafe { + std::env::set_var("PATH", format!("{}:{orig_path}", bin_dir.display())); + } + + let code = run(redirect_args(tmp.path(), server.uri())).await; + + unsafe { + std::env::set_var("PATH", orig_path); + } + assert_eq!(code, 1, "a corrupt ledger must flip the exit code"); + assert_eq!( + std::fs::read(tmp.path().join("bun.lockb")).ok().as_deref(), + Some(b"BUN-BINARY-PLACEHOLDER".as_slice()), + "the binary lock must be byte-untouched: the refusal precedes the migration" + ); + assert!( + !tmp.path().join("bun.lock").exists(), + "no text lock may be created by a run that refused before redirecting" + ); + // The malformed ledger is quarantined (never deleted), so recovery of the + // pre-redirect originals it may still hold stays possible. + let quarantined = tmp + .path() + .join(".socket/vendor/redirect-state.json.corrupt"); + assert_eq!( + std::fs::read(&quarantined).unwrap(), + corrupt_bytes, + "the malformed ledger is moved aside verbatim" + ); +} + /// An unusable ledger is an ERROR, not a silent success: /// `.socket/vendor/redirect-state.json` is the only revert path (and the VEX /// record store). A DIRECTORY squatting on the ledger path makes it @@ -1321,6 +1409,69 @@ async fn unwritable_ledger_fails_the_run() { ); } +/// Findings hosted-atomicity 1+2: a MID-RUN lockfile write failure (second of +/// two locks unwritable) must never leave the successfully-written first lock +/// redirected with no ledger record of its pre-redirect originals. The ledger +/// is persisted BEFORE the lockfile loop, so every planned edit's original is +/// durable even when a later write fails; the failed lock itself stays +/// byte-untouched (atomic stage+rename, no truncation). +/// +/// unix-only: a read-only directory does not block file creation on Windows. +#[cfg(unix)] +#[tokio::test] +#[serial] +async fn partial_lockfile_write_failure_persists_ledger_originals() { + use std::os::unix::fs::PermissionsExt; + + let server = MockServer::start().await; + mock_discovery(&server).await; + mock_reference(&server).await; + mock_view(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + // Rush repo: two pnpm locks, both resolving the patched package. The + // rewriter output map is a BTreeMap, so the common lock + // (common/config/rush/…) is written before the subspace lock + // (common/config/subspaces/…). + write_rush_project(tmp.path(), false); + let subspace_dir = tmp.path().join("common/config/subspaces/frontend"); + let before_subspace = std::fs::read_to_string(subspace_dir.join("pnpm-lock.yaml")).unwrap(); + std::fs::set_permissions(&subspace_dir, std::fs::Permissions::from_mode(0o555)).unwrap(); + + let code = run(redirect_args(tmp.path(), server.uri())).await; + + std::fs::set_permissions(&subspace_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!(code, 1, "a mid-run lockfile write failure must exit 1"); + + // The first lock landed before the failure… + let common = + std::fs::read_to_string(tmp.path().join("common/config/rush/pnpm-lock.yaml")).unwrap(); + assert!( + common.contains(HOSTED_URL), + "the common lock was written before the subspace failure; got:\n{common}" + ); + // …so its pre-redirect originals MUST already be in the ledger: without + // them a revert is impossible, and a re-run cannot recapture them (the + // entry is already redirected and produces no new edit). + let ledger = std::fs::read_to_string(tmp.path().join(".socket/vendor/redirect-state.json")) + .expect("the ledger must be persisted before any lockfile is mutated"); + assert!( + ledger.contains("UPSTREAMupstream"), + "the ledger must record the pre-redirect original integrity: {ledger}" + ); + assert!( + ledger.contains("common/config/rush/pnpm-lock.yaml"), + "the ledger must record the edit for the lock that WAS written: {ledger}" + ); + + // The failed lock is byte-untouched — no partial/truncated write. + assert_eq!( + std::fs::read_to_string(subspace_dir.join("pnpm-lock.yaml")).unwrap(), + before_subspace, + "the unwritable lock must stay byte-identical (atomic writes)" + ); +} + // ── Rush monorepo ──────────────────────────────────────────────────────── /// A Rush pnpm lock (v9) resolving the patched package under `packages:`, so @@ -2147,27 +2298,34 @@ async fn run_hosted_json_scan(tmp: &std::path::Path, server: &MockServer) -> std } /// Leg 3 of the four `--json` failure exits: the rewritten lockfile cannot -/// be written back (read-only file; the rewriter read it fine moments -/// earlier). A real filesystem obstruction drives the run to the write in -/// question and fails it there. (Legs 1-2 — the discovery-detail and -/// reference-resolve failures — are pinned by +/// be written back — its DIRECTORY is read-only, so the atomic stage file +/// cannot be created (the rewriter read the lock fine moments earlier). A +/// read-only lock FILE no longer fails this leg: the atomic stage+rename +/// replaces it mode-preserved, like the vendored backend's writer. (Legs 1-2 +/// — the discovery-detail and reference-resolve failures — are pinned by /// `redirect_json_mode_failures_emit_error_envelope` above; leg 4 — the /// ledger write — is pinned by the unix-only /// `redirect_ledger_write_failure_leaves_project_files_untouched` below.) +/// +/// unix-only: a read-only directory does not block file creation on Windows. +#[cfg(unix)] #[tokio::test] #[serial] async fn redirect_json_mode_write_failures_emit_error_envelope() { + use std::os::unix::fs::PermissionsExt; + let server = MockServer::start().await; mock_discovery(&server).await; mock_reference(&server).await; mock_view(&server).await; let tmp = tempfile::tempdir().unwrap(); - write_project(tmp.path()); - let lock = tmp.path().join("package-lock.json"); - let mut perms = std::fs::metadata(&lock).unwrap().permissions(); - perms.set_readonly(true); - std::fs::set_permissions(&lock, perms).unwrap(); + // Rush project: the lock lives in a subdirectory, so obstructing it does + // not also block the (earlier) `.socket/vendor` ledger write at the root. + write_rush_project(tmp.path(), false); + let lock_dir = tmp.path().join("common/config/rush"); + std::fs::set_permissions(&lock_dir, std::fs::Permissions::from_mode(0o555)).unwrap(); let out = run_hosted_json_scan(tmp.path(), &server).await; + std::fs::set_permissions(&lock_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); assert_write_failure_envelope(&out, "lockfile-write failure"); } @@ -2179,8 +2337,7 @@ async fn redirect_json_mode_write_failures_emit_error_envelope() { /// /// unix-only: the obstruction is a read-only DIRECTORY, and Windows ignores /// FILE_ATTRIBUTE_READONLY on directories for file creation, so the stage -/// file would be created fine there (leg 3's read-only FILE does obstruct on -/// Windows and stays cross-platform). +/// file would be created fine there. #[cfg(unix)] #[tokio::test] #[serial] diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index ddf3f9ec..e5c97d3f 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -98,7 +98,10 @@ pub struct DepOverride { /// One recorded file edit (mirrors the TS `FileEdit`). `Deserialize` so the /// persisted `redirect-state.json` ledger round-trips (see `redirect::state`). -#[derive(Debug, Clone, Serialize, Deserialize)] +/// `PartialEq` so the ledger merge can skip byte-identical edits a retried +/// run re-plans (the ledger persists BEFORE the lockfile writes, so a +/// failed write's edit is re-planned by the retry). +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct FileEdit { pub path: String, pub kind: String, diff --git a/crates/socket-patch-core/src/utils/fs.rs b/crates/socket-patch-core/src/utils/fs.rs index 020d0472..bcf19bda 100644 --- a/crates/socket-patch-core/src/utils/fs.rs +++ b/crates/socket-patch-core/src/utils/fs.rs @@ -189,8 +189,9 @@ pub(crate) async fn atomic_write_bytes(path: &Path, content: &[u8]) -> std::io:: /// this variant for files the *user* owns and we merely edit (package.json, /// Gemfile, …), matching npm's write-file-atomic. The patch engine keeps the /// plain writer: `restore_file_permissions` re-applies pre-patch mode + uid/gid -/// itself after the rename. -pub(crate) async fn atomic_write_bytes_preserving_mode( +/// itself after the rename. `pub` (not `pub(crate)`): the CLI's hosted +/// redirect writes user-owned lockfiles through it too. +pub async fn atomic_write_bytes_preserving_mode( path: &Path, content: &[u8], ) -> std::io::Result<()> {