From 8da8f386c9db1c46f876d0714acd3af8377bb09e Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 2 Jun 2026 15:40:48 -0400 Subject: [PATCH 1/3] feat(setup): add --check and --remove flags with dry-run + edit previews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `setup` previously only added its install hook. This adds two inverse modes, both honoring the existing --dry-run / --yes / --json flags and printing the exact set of proposed edits before touching disk: - `setup --check`: read-only verification. Exits 0 only when every package.json is configured for socket-patch and none failed to parse; otherwise exit 1. - `setup --remove`: reverts the lifecycle scripts setup added. Full revert — emptied postinstall/dependencies keys are deleted, an emptied `scripts` object is dropped, sibling scripts and key order are preserved. Scope is npm-family only, matching the surface `setup` already configures; check/remove operate purely on package.json. Core: `remove_socket_patch_from_script` + `remove_package_json{_object,_content}` in detect.rs (exact inverse of generate_updated_script, reusing the same non-object guards) and `remove_package_json` in update.rs. Tests: core unit tests; setup_invariants check/remove flows; cli_parse_setup flag + conflict coverage; and the setup-matrix driver now does a non-destructive check->remove->check round-trip after install/verify, asserted for npm-family cases (untagged elsewhere, consistent with the non-blocking BASELINE GAP convention). Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/socket-patch-cli/src/commands/setup.rs | 418 +++++++++++++++++- .../socket-patch-cli/tests/cli_parse_setup.rs | 33 ++ .../tests/setup_invariants.rs | 140 ++++++ .../tests/setup_matrix_common/mod.rs | 57 +++ .../src/package_json/detect.rs | 289 ++++++++++++ .../src/package_json/update.rs | 177 +++++++- tests/setup_matrix/run-case.sh | 34 +- 7 files changed, 1124 insertions(+), 24 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/setup.rs b/crates/socket-patch-cli/src/commands/setup.rs index 5a42cfdc..aee07ad9 100644 --- a/crates/socket-patch-cli/src/commands/setup.rs +++ b/crates/socket-patch-cli/src/commands/setup.rs @@ -1,9 +1,11 @@ use clap::Args; -use socket_patch_core::package_json::detect::PackageManager; +use socket_patch_core::package_json::detect::{is_setup_configured_str, PackageManager}; use socket_patch_core::package_json::find::{ - detect_package_manager, find_package_json_files, WorkspaceType, + detect_package_manager, find_package_json_files, PackageJsonLocation, WorkspaceType, +}; +use socket_patch_core::package_json::update::{ + remove_package_json, update_package_json, RemoveStatus, UpdateStatus, }; -use socket_patch_core::package_json::update::{update_package_json, UpdateStatus}; use socket_patch_core::utils::telemetry::track_patch_setup; use std::io::{self, Write}; use std::path::Path; @@ -21,24 +23,48 @@ fn manager_name(pm: PackageManager) -> &'static str { #[derive(Args)] pub struct SetupArgs { + /// Verify the project is configured for socket-patch without changing + /// anything. Exits non-zero if any package.json still needs setup. + #[arg( + long = "check", + conflicts_with = "remove", + default_value_t = false, + value_parser = clap::builder::BoolishValueParser::new(), + )] + pub check: bool, + + /// Revert the install hooks that `setup` added to package.json. + #[arg( + long = "remove", + default_value_t = false, + value_parser = clap::builder::BoolishValueParser::new(), + )] + pub remove: bool, + #[command(flatten)] pub common: GlobalArgs, } pub async fn run(args: SetupArgs) -> i32 { - if !args.common.json { - println!("Searching for package.json files..."); + if args.check { + run_check(&args).await + } else if args.remove { + run_remove(&args).await + } else { + run_setup(&args).await } +} +/// Discover the package.json files `setup`/`check`/`remove` should act on, +/// applying the pnpm "root-only" filtering. Returns `None` when no files are +/// found (the caller emits the `no_files` result). +async fn discover(args: &SetupArgs) -> Option> { let find_result = find_package_json_files(&args.common.cwd).await; - // For pnpm monorepos, only update root package.json. - // pnpm runs root postinstall on `pnpm install`, so workspace-level - // postinstall scripts are unnecessary. Individual workspaces may not - // have `@socketsecurity/socket-patch` as a dependency, causing - // `npx @socketsecurity/socket-patch apply` to fail due to pnpm's - // strict module isolation. - let package_json_files = match find_result.workspace_type { + // For pnpm monorepos, only update root package.json. pnpm runs root + // postinstall on `pnpm install`, so workspace-level postinstall scripts are + // unnecessary and would fail under pnpm's strict module isolation. + let files = match find_result.workspace_type { WorkspaceType::Pnpm => find_result .files .into_iter() @@ -47,21 +73,369 @@ pub async fn run(args: SetupArgs) -> i32 { _ => find_result.files, }; - if package_json_files.is_empty() { - if args.common.json { - println!("{}", serde_json::to_string_pretty(&serde_json::json!({ - "status": "no_files", - "updated": 0, - "alreadyConfigured": 0, - "errors": 0, + if files.is_empty() { + None + } else { + Some(files) + } +} + +/// Emit the shared "no package.json files found" result and exit code. +fn report_no_files(args: &SetupArgs, status: &str) -> i32 { + if args.common.json { + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "status": status, "files": [], - })).unwrap()); + })) + .unwrap() + ); + } else { + println!("No package.json files found"); + } + 0 +} + +// ───────────────────────────────────────────────────────────────────────── +// check +// ───────────────────────────────────────────────────────────────────────── + +/// Read-only verification that every discovered package.json is configured for +/// socket-patch. Never writes (so `--dry-run` is a harmless no-op here). Exits +/// 0 only when all files are configured and none failed to parse. +async fn run_check(args: &SetupArgs) -> i32 { + if !args.common.json { + println!("Searching for package.json files..."); + } + + let files = match discover(args).await { + Some(f) => f, + None => return report_no_files(args, "no_files"), + }; + + #[derive(Clone, Copy, PartialEq)] + enum CheckState { + Configured, + NeedsConfiguration, + Error, + } + + let mut entries = Vec::new(); + for loc in &files { + let (state, err) = match tokio::fs::read_to_string(&loc.path).await { + Ok(content) => { + // A malformed package.json cannot be verified; surface it as an + // error rather than silently "needs configuration". + if serde_json::from_str::(&content).is_err() { + (CheckState::Error, Some("Invalid package.json".to_string())) + } else if is_setup_configured_str(&content).needs_update { + (CheckState::NeedsConfiguration, None) + } else { + (CheckState::Configured, None) + } + } + Err(e) => (CheckState::Error, Some(e.to_string())), + }; + entries.push((loc.path.display().to_string(), state, err)); + } + + let configured = entries.iter().filter(|(_, s, _)| *s == CheckState::Configured).count(); + let needs = entries.iter().filter(|(_, s, _)| *s == CheckState::NeedsConfiguration).count(); + let errs = entries.iter().filter(|(_, s, _)| *s == CheckState::Error).count(); + + let all_ok = needs == 0 && errs == 0; + let status = if errs > 0 { + "error" + } else if all_ok { + "configured" + } else { + "needs_configuration" + }; + + if args.common.json { + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "status": status, + "configured": configured, + "needsConfiguration": needs, + "errors": errs, + "files": entries.iter().map(|(path, state, err)| { + serde_json::json!({ + "path": path, + "status": match state { + CheckState::Configured => "configured", + CheckState::NeedsConfiguration => "needs_configuration", + CheckState::Error => "error", + }, + "error": err, + }) + }).collect::>(), + })) + .unwrap() + ); + } else { + println!("\nConfiguration status:\n"); + for (path, state, err) in &entries { + let rel = pathdiff(path, &args.common.cwd); + match state { + CheckState::Configured => println!(" ✓ {rel} (configured)"), + CheckState::NeedsConfiguration => println!(" ✗ {rel} (needs setup)"), + CheckState::Error => println!( + " ! {rel}: {}", + err.as_deref().unwrap_or("unknown error") + ), + } + } + println!(); + if all_ok { + println!("All package.json files are configured with socket-patch."); } else { - println!("No package.json files found"); + println!( + "{needs} file(s) need configuration, {errs} error(s). Run `socket-patch setup` to fix." + ); } - return 0; } + if all_ok { + 0 + } else { + 1 + } +} + +// ───────────────────────────────────────────────────────────────────────── +// remove +// ───────────────────────────────────────────────────────────────────────── + +/// Render a removed script value: `None` means the key is being deleted. +fn render_removed(new: &Option) -> String { + match new { + Some(s) if !s.is_empty() => format!("\"{s}\""), + _ => "(removed)".to_string(), + } +} + +/// Revert the install hooks `setup` added. Honors `--dry-run` (preview only), +/// `--yes` (skip confirmation), and `--json`. +async fn run_remove(args: &SetupArgs) -> i32 { + if !args.common.json { + println!("Searching for package.json files..."); + } + + let files = match discover(args).await { + Some(f) => f, + None => return report_no_files(args, "no_files"), + }; + + if !args.common.json { + println!("Found {} package.json file(s)", files.len()); + } + + // Preview every file (dry_run=true never writes). + let mut preview = Vec::new(); + for loc in &files { + preview.push(remove_package_json(&loc.path, true).await); + } + + let to_remove: Vec<_> = preview.iter().filter(|r| r.status == RemoveStatus::Removed).collect(); + let not_configured: Vec<_> = + preview.iter().filter(|r| r.status == RemoveStatus::NotConfigured).collect(); + let errors: Vec<_> = preview.iter().filter(|r| r.status == RemoveStatus::Error).collect(); + + // Display proposed edits (human mode). + if !args.common.json { + println!("\nProposed changes:\n"); + if !to_remove.is_empty() { + println!("Will remove socket-patch from:"); + for r in &to_remove { + let rel = pathdiff(&r.path, &args.common.cwd); + println!(" - {rel}"); + println!(" postinstall: \"{}\"", r.old_script); + println!(" -> postinstall: {}", render_removed(&r.new_script)); + println!(" dependencies: \"{}\"", r.old_dependencies_script); + println!( + " -> dependencies: {}", + render_removed(&r.new_dependencies_script) + ); + } + println!(); + } + if !not_configured.is_empty() { + println!("Nothing to remove (will skip):"); + for r in ¬_configured { + println!(" = {}", pathdiff(&r.path, &args.common.cwd)); + } + println!(); + } + if !errors.is_empty() { + println!("Errors:"); + for r in &errors { + println!( + " ! {}: {}", + pathdiff(&r.path, &args.common.cwd), + r.error.as_deref().unwrap_or("unknown error") + ); + } + println!(); + } + } + + let json_files = |results: &[&socket_patch_core::package_json::update::RemoveResult]| { + results + .iter() + .map(|r| { + serde_json::json!({ + "path": r.path, + "status": match r.status { + RemoveStatus::Removed => "removed", + RemoveStatus::NotConfigured => "not_configured", + RemoveStatus::Error => "error", + }, + "error": r.error, + }) + }) + .collect::>() + }; + + // Nothing to remove: either everything is already clean (exit 0) or some + // file errored (exit 1). Mirrors the setup flow's honest error handling. + if to_remove.is_empty() { + let errs = errors.len(); + if args.common.json { + let all: Vec<_> = preview.iter().collect(); + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "status": if errs > 0 { "error" } else { "not_configured" }, + "removed": 0, + "notConfigured": not_configured.len(), + "errors": errs, + "files": json_files(&all), + })) + .unwrap() + ); + } else if errs > 0 { + println!("Nothing removed; {errs} file(s) could not be processed (see errors above)."); + } else { + println!("No socket-patch install hooks found to remove."); + } + return if errs > 0 { 1 } else { 0 }; + } + + // Dry-run: preview already shown; report and exit without writing. + if args.common.dry_run { + if args.common.json { + let all: Vec<_> = preview.iter().collect(); + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "status": "dry_run", + "wouldRemove": to_remove.len(), + "notConfigured": not_configured.len(), + "errors": errors.len(), + "dryRun": true, + "files": json_files(&all), + })) + .unwrap() + ); + } else { + println!("\nSummary:"); + println!(" {} file(s) would have socket-patch removed", to_remove.len()); + println!(" {} file(s) have nothing to remove", not_configured.len()); + if !errors.is_empty() { + println!(" {} error(s)", errors.len()); + } + } + return if errors.is_empty() { 0 } else { 1 }; + } + + // Confirm before mutating. + if !args.common.yes && !args.common.json { + if !stdin_is_tty() { + eprintln!("Non-interactive mode detected, proceeding automatically."); + } else { + print!("Remove these install hooks? (y/N): "); + io::stdout().flush().unwrap(); + let mut answer = String::new(); + io::stdin().read_line(&mut answer).unwrap(); + let answer = answer.trim().to_lowercase(); + if answer != "y" && answer != "yes" { + println!("Aborted"); + return 0; + } + } + } + + if !args.common.json { + println!("\nRemoving changes..."); + } + let mut results = Vec::new(); + for loc in &files { + results.push(remove_package_json(&loc.path, false).await); + } + + let removed = results.iter().filter(|r| r.status == RemoveStatus::Removed).count(); + let not_cfg = results.iter().filter(|r| r.status == RemoveStatus::NotConfigured).count(); + let errs = results.iter().filter(|r| r.status == RemoveStatus::Error).count(); + + if args.common.json { + let all: Vec<_> = results.iter().collect(); + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "status": if errs > 0 { "partial_failure" } else { "success" }, + "removed": removed, + "notConfigured": not_cfg, + "errors": errs, + "files": json_files(&all), + })) + .unwrap() + ); + } else { + println!("\nSummary:"); + println!(" {removed} file(s) had socket-patch removed"); + println!(" {not_cfg} file(s) had nothing to remove"); + if errs > 0 { + println!(" {errs} error(s)"); + } + } + + if errs > 0 { + 1 + } else { + 0 + } +} + +// ───────────────────────────────────────────────────────────────────────── +// setup (unchanged behavior) +// ───────────────────────────────────────────────────────────────────────── + +async fn run_setup(args: &SetupArgs) -> i32 { + if !args.common.json { + println!("Searching for package.json files..."); + } + + let package_json_files = match discover(args).await { + Some(f) => f, + None => { + if args.common.json { + println!("{}", serde_json::to_string_pretty(&serde_json::json!({ + "status": "no_files", + "updated": 0, + "alreadyConfigured": 0, + "errors": 0, + "files": [], + })).unwrap()); + } else { + println!("No package.json files found"); + } + return 0; + } + }; + // Detect package manager from lockfiles in the project root. let pm = detect_package_manager(&args.common.cwd).await; diff --git a/crates/socket-patch-cli/tests/cli_parse_setup.rs b/crates/socket-patch-cli/tests/cli_parse_setup.rs index 3de483d9..da50e830 100644 --- a/crates/socket-patch-cli/tests/cli_parse_setup.rs +++ b/crates/socket-patch-cli/tests/cli_parse_setup.rs @@ -74,6 +74,37 @@ fn json_long_form() { assert!(args.common.json); } +#[test] +fn check_long_form() { + let args = parse_setup(&["--check"]); + assert!(args.check); + assert!(!args.remove); +} + +#[test] +fn remove_long_form() { + let args = parse_setup(&["--remove"]); + assert!(args.remove); + assert!(!args.check); +} + +#[test] +fn check_and_remove_conflict() { + let result = Cli::try_parse_from(["socket-patch", "setup", "--check", "--remove"]); + let err = match result { + Ok(_) => panic!("--check + --remove must conflict"), + Err(e) => e, + }; + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); +} + +#[test] +fn defaults_check_and_remove_false() { + let args = parse_setup(&[]); + assert!(!args.check); + assert!(!args.remove); +} + #[test] fn all_flags_combined() { let args = parse_setup(&["--cwd", "/tmp/x", "--dry-run", "-y", "--json"]); @@ -105,6 +136,8 @@ fn unknown_flag_is_error() { async fn run_empty_tempdir_exits_zero() { let tempdir = tempfile::tempdir().expect("tempdir"); let args = SetupArgs { + check: false, + remove: false, common: socket_patch_cli::args::GlobalArgs { cwd: tempdir.path().to_path_buf(), dry_run: false, diff --git a/crates/socket-patch-cli/tests/setup_invariants.rs b/crates/socket-patch-cli/tests/setup_invariants.rs index 39b5c552..97136776 100644 --- a/crates/socket-patch-cli/tests/setup_invariants.rs +++ b/crates/socket-patch-cli/tests/setup_invariants.rs @@ -334,3 +334,143 @@ fn setup_partial_failure_exits_nonzero_when_applying() { let root = std::fs::read_to_string(tmp.path().join("package.json")).unwrap(); assert!(root.contains("socket-patch"), "valid file should still be updated"); } + +// --------------------------------------------------------------------------- +// `setup --check` — read-only verification +// --------------------------------------------------------------------------- + +#[test] +fn setup_check_configured_project_exits_zero() { + let tmp = tempfile::tempdir().expect("tempdir"); + let pkg = tmp.path().join("package.json"); + write(&pkg, r#"{ "name": "x", "version": "1.0.0" }"#); + // Configure it first. + let (c, _) = run_setup(tmp.path(), &["--yes"]); + assert_eq!(c, 0); + + let (code, stdout) = run_setup(tmp.path(), &["--check"]); + assert_eq!(code, 0, "configured project should pass --check; stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["status"], "configured"); + assert_eq!(v["needsConfiguration"], 0); +} + +#[test] +fn setup_check_unconfigured_project_exits_nonzero() { + let tmp = tempfile::tempdir().expect("tempdir"); + write(&tmp.path().join("package.json"), r#"{ "name": "x", "scripts": { "build": "tsc" } }"#); + + let (code, stdout) = run_setup(tmp.path(), &["--check"]); + assert_eq!(code, 1, "unconfigured project must fail --check; stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["status"], "needs_configuration"); + assert_eq!(v["needsConfiguration"], 1); +} + +#[test] +fn setup_check_no_files_exits_zero() { + let tmp = tempfile::tempdir().expect("tempdir"); + let (code, stdout) = run_setup(tmp.path(), &["--check"]); + assert_eq!(code, 0, "no files should still exit 0; stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["status"], "no_files"); +} + +#[test] +fn setup_check_does_not_modify_file() { + let tmp = tempfile::tempdir().expect("tempdir"); + let pkg = tmp.path().join("package.json"); + let original = "{ \"name\": \"x\", \"scripts\": { \"build\": \"tsc\" } }"; + write(&pkg, original); + run_setup(tmp.path(), &["--check"]); + assert_eq!( + std::fs::read_to_string(&pkg).unwrap(), + original, + "--check must never write" + ); +} + +// --------------------------------------------------------------------------- +// `setup --remove` — revert the install hooks +// --------------------------------------------------------------------------- + +#[test] +fn setup_remove_round_trips_and_preserves_other_scripts() { + let tmp = tempfile::tempdir().expect("tempdir"); + let pkg = tmp.path().join("package.json"); + write(&pkg, r#"{ "name": "x", "scripts": { "build": "tsc" } }"#); + + // Configure, then remove. + let (c1, _) = run_setup(tmp.path(), &["--yes"]); + assert_eq!(c1, 0); + let after_setup = std::fs::read_to_string(&pkg).unwrap(); + assert!(after_setup.contains("socket-patch")); + + let (code, stdout) = run_setup(tmp.path(), &["--remove", "--yes"]); + assert_eq!(code, 0, "remove should succeed; stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["status"], "success"); + assert_eq!(v["removed"], 1); + + let after = std::fs::read_to_string(&pkg).unwrap(); + assert!(!after.contains("socket-patch"), "socket-patch must be gone; got:\n{after}"); + let parsed: serde_json::Value = serde_json::from_str(&after).expect("valid JSON"); + // Full revert: lifecycle keys gone, sibling script preserved. + assert_eq!(parsed["scripts"]["build"], "tsc"); + assert!(parsed["scripts"].get("postinstall").is_none()); + assert!(parsed["scripts"].get("dependencies").is_none()); + + // And --check now reports it needs configuration again. + let (c2, _) = run_setup(tmp.path(), &["--check"]); + assert_eq!(c2, 1, "after remove, --check must fail again"); +} + +#[test] +fn setup_remove_dry_run_does_not_modify_file() { + let tmp = tempfile::tempdir().expect("tempdir"); + let pkg = tmp.path().join("package.json"); + write(&pkg, r#"{ "name": "x", "version": "1.0.0" }"#); + let (c1, _) = run_setup(tmp.path(), &["--yes"]); + assert_eq!(c1, 0); + let configured = std::fs::read_to_string(&pkg).unwrap(); + + let (code, stdout) = run_setup(tmp.path(), &["--remove", "--dry-run"]); + assert_eq!(code, 0, "remove dry-run should succeed; stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["status"], "dry_run"); + assert_eq!(v["dryRun"], true); + assert_eq!(v["wouldRemove"], 1); + + assert_eq!( + std::fs::read_to_string(&pkg).unwrap(), + configured, + "remove --dry-run must not modify package.json" + ); +} + +#[test] +fn setup_remove_nothing_to_remove_exits_zero() { + let tmp = tempfile::tempdir().expect("tempdir"); + write(&tmp.path().join("package.json"), r#"{ "name": "x", "scripts": { "build": "tsc" } }"#); + + let (code, stdout) = run_setup(tmp.path(), &["--remove", "--yes"]); + assert_eq!(code, 0, "nothing to remove should exit 0; stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["status"], "not_configured"); + assert_eq!(v["removed"], 0); +} + +#[test] +fn setup_check_and_remove_are_mutually_exclusive() { + let tmp = tempfile::tempdir().expect("tempdir"); + write(&tmp.path().join("package.json"), r#"{ "name": "x" }"#); + + // clap conflict → usage error (exit 2), not a normal run. + let out = Command::new(binary()) + .args(["setup", "--check", "--remove"]) + .current_dir(tmp.path()) + .env_remove("SOCKET_API_TOKEN") + .output() + .expect("run socket-patch"); + assert_ne!(out.status.code(), Some(0), "--check + --remove must be rejected"); +} diff --git a/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs b/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs index d2fe02c1..d9b1c9b5 100644 --- a/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs +++ b/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs @@ -106,6 +106,13 @@ impl Case { self.expect_applied && self.baseline_supported } + /// npm-family package managers (plus the polyglot monorepo's npm slice) + /// are the surface `setup` actually configures today — the only cases + /// where the check/remove round-trip is expected to do real work. + fn is_npm_family(&self) -> bool { + matches!(self.pm.as_str(), "npm" | "yarn" | "pnpm" | "bun") || self.layout == "monorepo" + } + fn sm_env(&self) -> Vec<(String, String)> { vec![ ("SM_ID".into(), self.id.clone()), @@ -298,6 +305,16 @@ fn run_cases(label: &str, cases: Vec) { case.id, case.expect_applied, res.actual_applied, tag, indent(&res.raw) )); } + + // check/remove round-trip — only asserted for npm-family cases that + // ran setup (the surface setup configures today). For other + // ecosystems setup writes nothing, so the round-trip is a no-op and + // we leave it untagged, consistent with the BASELINE GAP convention. + if case.run_setup && case.is_npm_family() { + if let Some(msg) = round_trip_failure(case, &res) { + failures.push(msg); + } + } } assert!( @@ -313,6 +330,46 @@ fn run_cases(label: &str, cases: Vec) { ); } +/// Validate the `setup --check` / `setup --remove` round-trip fields emitted by +/// the driver. Returns a failure message if the inverse of setup did not hold: +/// after configuring, `--check` must pass (0); `--remove` must succeed (0) and +/// strip socket-patch from package.json; and `--check` must then fail again +/// (non-zero). Returns `None` on success. +fn round_trip_failure(case: &Case, res: &RunResult) -> Option { + let parsed = res.parsed.as_ref()?; + let int = |k: &str| parsed.get(k).and_then(|v| v.as_i64()); + let boolean = |k: &str| parsed.get(k).and_then(|v| v.as_bool()); + + let check_setup = int("check_after_setup_exit"); + let remove = int("remove_exit"); + let check_remove = int("check_after_remove_exit"); + let clean = boolean("remove_clean"); + + let mut problems = Vec::new(); + if check_setup != Some(0) { + problems.push(format!("check-after-setup exit={check_setup:?} (want 0)")); + } + if remove != Some(0) { + problems.push(format!("remove exit={remove:?} (want 0)")); + } + if check_remove == Some(0) { + problems.push("check-after-remove exit=0 (want non-zero; hook still present)".to_string()); + } + if clean == Some(false) { + problems.push("socket-patch still present in package.json after remove".to_string()); + } + + if problems.is_empty() { + return None; + } + Some(format!( + " - {}: check/remove round-trip failed [{}]\n{}", + case.id, + problems.join("; "), + indent(&res.raw) + )) +} + fn indent(s: &str) -> String { s.lines().map(|l| format!(" {l}")).collect::>().join("\n") } diff --git a/crates/socket-patch-core/src/package_json/detect.rs b/crates/socket-patch-core/src/package_json/detect.rs index 2e499e6d..3719c735 100644 --- a/crates/socket-patch-core/src/package_json/detect.rs +++ b/crates/socket-patch-core/src/package_json/detect.rs @@ -154,6 +154,171 @@ pub fn update_package_json_object( (modified, new_postinstall, new_dependencies) } +/// Strip every socket-patch segment out of a single lifecycle script. +/// +/// Scripts are joined with `" && "` (that is exactly how +/// [`generate_updated_script`] prepends the patch command), so splitting on +/// the same separator and dropping any segment that is a socket-patch invocation +/// reverses the setup edit, whether the command was added to an empty script +/// (`""`) or prepended to an existing one (`" && build"`). +/// +/// Returns `(changed, new_value)`: +/// - `(false, Some(original))` — no socket-patch segment found; leave as-is. +/// - `(true, Some(rest))` — patch segment(s) removed, other commands survive. +/// - `(true, None)` — the script was *only* socket-patch; the key should be +/// deleted entirely. +pub fn remove_socket_patch_from_script(script: &str) -> (bool, Option) { + let trimmed = script.trim(); + if trimmed.is_empty() { + return (false, None); + } + + let segments: Vec<&str> = trimmed.split(" && ").collect(); + let kept: Vec<&str> = segments + .iter() + .map(|s| s.trim()) + .filter(|s| !s.is_empty() && !script_is_configured(s)) + .collect(); + + if kept.len() == segments.len() { + // Nothing matched a socket-patch pattern (and no empty segments) — + // unchanged. + return (false, Some(trimmed.to_string())); + } + + if kept.is_empty() { + (true, None) + } else { + (true, Some(kept.join(" && "))) + } +} + +/// Status of a remove operation on a single package.json object. +#[derive(Debug, Clone)] +pub struct ScriptRemoveStatus { + pub modified: bool, + pub old_postinstall: String, + pub new_postinstall: Option, + pub old_dependencies: String, + pub new_dependencies: Option, +} + +/// Remove socket-patch from both lifecycle scripts in a package.json object. +/// +/// Full revert: an emptied `postinstall`/`dependencies` key is deleted, and if +/// `scripts` ends up empty the whole `scripts` key is dropped too — undoing +/// exactly what [`update_package_json_object`] added. Returns a +/// [`ScriptRemoveStatus`] describing what changed. +pub fn remove_package_json_object(package_json: &mut serde_json::Value) -> ScriptRemoveStatus { + let read_script = |pj: &serde_json::Value, key: &str| -> String { + pj.get("scripts") + .and_then(|s| s.get(key)) + .and_then(|v| v.as_str()) + .unwrap_or("") + .to_string() + }; + + let old_postinstall = read_script(package_json, "postinstall"); + let old_dependencies = read_script(package_json, "dependencies"); + + let (pi_changed, new_postinstall) = remove_socket_patch_from_script(&old_postinstall); + let (dep_changed, new_dependencies) = remove_socket_patch_from_script(&old_dependencies); + + // Only treat as modified when a socket-patch segment was actually present. + let pi_had_patch = pi_changed && script_is_configured(&old_postinstall); + let dep_had_patch = dep_changed && script_is_configured(&old_dependencies); + let modified = pi_had_patch || dep_had_patch; + + if !modified { + return ScriptRemoveStatus { + modified: false, + new_postinstall: Some(old_postinstall.clone()), + old_postinstall, + new_dependencies: Some(old_dependencies.clone()), + old_dependencies, + }; + } + + // We can only mutate scripts on an object root with an object `scripts`. + // Anything else has nothing to remove and is handled by the no-op path + // above (its scripts read as empty). + if let Some(scripts) = package_json + .get_mut("scripts") + .and_then(|s| s.as_object_mut()) + { + if pi_had_patch { + match &new_postinstall { + Some(s) => { + scripts.insert( + "postinstall".to_string(), + serde_json::Value::String(s.clone()), + ); + } + None => { + scripts.remove("postinstall"); + } + } + } + if dep_had_patch { + match &new_dependencies { + Some(s) => { + scripts.insert( + "dependencies".to_string(), + serde_json::Value::String(s.clone()), + ); + } + None => { + scripts.remove("dependencies"); + } + } + } + + // If `scripts` is now empty, drop the key entirely for a clean revert. + if scripts.is_empty() { + if let Some(obj) = package_json.as_object_mut() { + obj.remove("scripts"); + } + } + } + + ScriptRemoveStatus { + modified, + old_postinstall, + new_postinstall, + old_dependencies, + new_dependencies, + } +} + +/// Parse package.json content and remove socket-patch lifecycle scripts. +/// Returns `(modified, new_content, status)`. +pub fn remove_package_json_content( + content: &str, +) -> Result<(bool, String, ScriptRemoveStatus), String> { + let mut package_json: serde_json::Value = + serde_json::from_str(content).map_err(|e| format!("Invalid package.json: {e}"))?; + + if !package_json.is_object() { + return Err("Invalid package.json: root is not a JSON object".to_string()); + } + + // Refuse to touch a malformed (present but non-object) `scripts` value. + if let Some(scripts) = package_json.get("scripts") { + if !scripts.is_null() && !scripts.is_object() { + return Err("Invalid package.json: \"scripts\" is not a JSON object".to_string()); + } + } + + let status = remove_package_json_object(&mut package_json); + + if !status.modified { + return Ok((false, content.to_string(), status)); + } + + let new_content = serde_json::to_string_pretty(&package_json).unwrap() + "\n"; + Ok((true, new_content, status)) +} + /// Parse package.json content and update it with socket-patch scripts. /// Returns (modified, new_content, old_postinstall, new_postinstall, /// old_dependencies, new_dependencies). @@ -546,6 +711,130 @@ mod tests { assert!(parsed["scripts"]["dependencies"].is_string()); } + // ── remove_socket_patch_from_script ───────────────────────────── + + #[test] + fn test_remove_script_only_socket_patch_deletes_key() { + let (changed, new) = remove_socket_patch_from_script( + "npx @socketsecurity/socket-patch apply --silent --ecosystems npm", + ); + assert!(changed); + assert_eq!(new, None); + } + + #[test] + fn test_remove_script_strips_prefix_keeps_rest() { + let (changed, new) = remove_socket_patch_from_script( + "npx @socketsecurity/socket-patch apply --silent --ecosystems npm && echo done", + ); + assert!(changed); + assert_eq!(new.as_deref(), Some("echo done")); + } + + #[test] + fn test_remove_script_no_socket_patch_unchanged() { + let (changed, new) = remove_socket_patch_from_script("echo done && tsc"); + assert!(!changed); + assert_eq!(new.as_deref(), Some("echo done && tsc")); + } + + #[test] + fn test_remove_script_legacy_pattern() { + let (changed, new) = remove_socket_patch_from_script("socket-patch apply && echo done"); + assert!(changed); + assert_eq!(new.as_deref(), Some("echo done")); + } + + #[test] + fn test_remove_script_empty() { + let (changed, new) = remove_socket_patch_from_script(""); + assert!(!changed); + assert_eq!(new, None); + } + + // ── remove_package_json_object ────────────────────────────────── + + #[test] + fn test_remove_object_deletes_lifecycle_keys() { + let mut pkg: serde_json::Value = serde_json::json!({ + "name": "test", + "scripts": { + "postinstall": "npx @socketsecurity/socket-patch apply --silent --ecosystems npm", + "dependencies": "npx @socketsecurity/socket-patch apply --silent --ecosystems npm" + } + }); + let status = remove_package_json_object(&mut pkg); + assert!(status.modified); + // Both keys were only socket-patch, so they (and the now-empty + // `scripts` object) are removed entirely. + assert!(pkg.get("scripts").is_none()); + } + + #[test] + fn test_remove_object_keeps_sibling_scripts() { + let mut pkg: serde_json::Value = serde_json::json!({ + "name": "test", + "scripts": { + "build": "tsc", + "postinstall": "npx @socketsecurity/socket-patch apply --silent --ecosystems npm && echo hi" + } + }); + let status = remove_package_json_object(&mut pkg); + assert!(status.modified); + assert_eq!(pkg["scripts"]["build"], "tsc"); + assert_eq!(pkg["scripts"]["postinstall"], "echo hi"); + } + + #[test] + fn test_remove_object_noop_when_not_configured() { + let mut pkg: serde_json::Value = serde_json::json!({ + "name": "test", + "scripts": { "build": "tsc" } + }); + let status = remove_package_json_object(&mut pkg); + assert!(!status.modified); + assert_eq!(pkg["scripts"]["build"], "tsc"); + } + + // ── remove_package_json_content ───────────────────────────────── + + #[test] + fn test_remove_content_roundtrip_with_update() { + // update then remove must return to a no-socket-patch state. + let original = r#"{"name":"x","scripts":{"build":"tsc"}}"#; + let (_, updated, ..) = + update_package_json_content(original, PackageManager::Npm).unwrap(); + assert!(updated.contains("socket-patch")); + + let (modified, removed, _) = remove_package_json_content(&updated).unwrap(); + assert!(modified); + assert!(!removed.contains("socket-patch")); + let parsed: serde_json::Value = serde_json::from_str(&removed).unwrap(); + assert_eq!(parsed["scripts"]["build"], "tsc"); + assert!(parsed["scripts"].get("postinstall").is_none()); + assert!(parsed["scripts"].get("dependencies").is_none()); + } + + #[test] + fn test_remove_content_idempotent() { + let configured = r#"{"name":"x","scripts":{"postinstall":"npx @socketsecurity/socket-patch apply"}}"#; + let (modified1, removed, _) = remove_package_json_content(configured).unwrap(); + assert!(modified1); + let (modified2, _, _) = remove_package_json_content(&removed).unwrap(); + assert!(!modified2); + } + + #[test] + fn test_remove_content_invalid_json_errors() { + assert!(remove_package_json_content("not json").is_err()); + } + + #[test] + fn test_remove_content_non_object_scripts_errors() { + let result = remove_package_json_content(r#"{"name":"x","scripts":"build"}"#); + assert!(result.is_err()); + } + #[test] fn test_update_content_pnpm() { let content = r#"{"name": "test"}"#; diff --git a/crates/socket-patch-core/src/package_json/update.rs b/crates/socket-patch-core/src/package_json/update.rs index 79afef33..f535de19 100644 --- a/crates/socket-patch-core/src/package_json/update.rs +++ b/crates/socket-patch-core/src/package_json/update.rs @@ -1,7 +1,10 @@ use std::path::Path; use tokio::fs; -use super::detect::{is_setup_configured_str, update_package_json_content, PackageManager}; +use super::detect::{ + is_setup_configured_str, remove_package_json_content, update_package_json_content, + PackageManager, +}; /// Result of updating a single package.json. #[derive(Debug, Clone)] @@ -108,6 +111,101 @@ pub async fn update_package_json( } } +/// Result of removing socket-patch from a single package.json. +#[derive(Debug, Clone)] +pub struct RemoveResult { + pub path: String, + pub status: RemoveStatus, + /// Previous `postinstall` script (empty if absent). + pub old_script: String, + /// New `postinstall` value: `None` means the key was deleted. + pub new_script: Option, + pub old_dependencies_script: String, + pub new_dependencies_script: Option, + pub error: Option, +} + +#[derive(Debug, Clone, PartialEq)] +pub enum RemoveStatus { + /// socket-patch was present and has been (or would be) removed. + Removed, + /// Nothing to remove — the file is not configured for socket-patch. + NotConfigured, + Error, +} + +/// Remove socket-patch lifecycle scripts from a single package.json file. +/// +/// Mirrors [`update_package_json`] but in reverse. Needs no [`PackageManager`]: +/// it strips any known socket-patch pattern regardless of how it was written. +pub async fn remove_package_json(package_json_path: &Path, dry_run: bool) -> RemoveResult { + let path_str = package_json_path.display().to_string(); + + let content = match fs::read_to_string(package_json_path).await { + Ok(c) => c, + Err(e) => { + return RemoveResult { + path: path_str, + status: RemoveStatus::Error, + old_script: String::new(), + new_script: None, + old_dependencies_script: String::new(), + new_dependencies_script: None, + error: Some(e.to_string()), + }; + } + }; + + match remove_package_json_content(&content) { + Ok((modified, new_content, status)) => { + if !modified { + return RemoveResult { + path: path_str, + status: RemoveStatus::NotConfigured, + old_script: status.old_postinstall, + new_script: status.new_postinstall, + old_dependencies_script: status.old_dependencies, + new_dependencies_script: status.new_dependencies, + error: None, + }; + } + + if !dry_run { + if let Err(e) = fs::write(package_json_path, &new_content).await { + return RemoveResult { + path: path_str, + status: RemoveStatus::Error, + old_script: status.old_postinstall, + new_script: status.new_postinstall, + old_dependencies_script: status.old_dependencies, + new_dependencies_script: status.new_dependencies, + error: Some(e.to_string()), + }; + } + } + + RemoveResult { + path: path_str, + status: RemoveStatus::Removed, + old_script: status.old_postinstall, + new_script: status.new_postinstall, + old_dependencies_script: status.old_dependencies, + new_dependencies_script: status.new_dependencies, + error: None, + } + } + Err(e) => RemoveResult { + path: path_str, + status: RemoveStatus::Error, + old_script: String::new(), + new_script: None, + old_dependencies_script: String::new(), + new_dependencies_script: None, + error: Some(e), + }, + } +} + #[cfg(test)] mod tests { use super::*; @@ -346,4 +444,81 @@ mod tests { assert!(result.new_script.contains("echo hi")); assert_eq!(fs::read_to_string(&pkg).await.unwrap(), original); } + + // ── remove_package_json ───────────────────────────────────────── + + #[tokio::test] + async fn test_remove_file_not_found() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("nonexistent.json"); + let result = remove_package_json(&missing, false).await; + assert_eq!(result.status, RemoveStatus::Error); + assert!(result.error.is_some()); + } + + #[tokio::test] + async fn test_remove_not_configured() { + let dir = tempfile::tempdir().unwrap(); + let pkg = dir.path().join("package.json"); + fs::write(&pkg, r#"{"name":"x","scripts":{"build":"tsc"}}"#) + .await + .unwrap(); + let result = remove_package_json(&pkg, false).await; + assert_eq!(result.status, RemoveStatus::NotConfigured); + } + + #[tokio::test] + async fn test_remove_writes_and_strips_socket_patch() { + let dir = tempfile::tempdir().unwrap(); + let pkg = dir.path().join("package.json"); + // Configure first, then remove. + fs::write(&pkg, r#"{"name":"x","scripts":{"build":"tsc"}}"#) + .await + .unwrap(); + update_package_json(&pkg, false, PackageManager::Npm).await; + + let result = remove_package_json(&pkg, false).await; + assert_eq!(result.status, RemoveStatus::Removed); + let content = fs::read_to_string(&pkg).await.unwrap(); + assert!(!content.contains("socket-patch")); + let parsed: serde_json::Value = serde_json::from_str(&content).unwrap(); + assert_eq!(parsed["scripts"]["build"], "tsc"); + } + + #[tokio::test] + async fn test_remove_dry_run_does_not_write() { + let dir = tempfile::tempdir().unwrap(); + let pkg = dir.path().join("package.json"); + let original = r#"{"name":"x","scripts":{"postinstall":"npx @socketsecurity/socket-patch apply"}}"#; + fs::write(&pkg, original).await.unwrap(); + let result = remove_package_json(&pkg, true).await; + assert_eq!(result.status, RemoveStatus::Removed); + // File must be byte-identical after a dry-run. + assert_eq!(fs::read_to_string(&pkg).await.unwrap(), original); + } + + #[tokio::test] + async fn test_remove_idempotent() { + let dir = tempfile::tempdir().unwrap(); + let pkg = dir.path().join("package.json"); + fs::write(&pkg, r#"{"name":"x","scripts":{"build":"tsc"}}"#) + .await + .unwrap(); + update_package_json(&pkg, false, PackageManager::Npm).await; + + let r1 = remove_package_json(&pkg, false).await; + assert_eq!(r1.status, RemoveStatus::Removed); + let r2 = remove_package_json(&pkg, false).await; + assert_eq!(r2.status, RemoveStatus::NotConfigured); + } + + #[tokio::test] + async fn test_remove_invalid_json_errors_and_leaves_file() { + let dir = tempfile::tempdir().unwrap(); + let pkg = dir.path().join("package.json"); + fs::write(&pkg, "not json!!!").await.unwrap(); + let result = remove_package_json(&pkg, false).await; + assert_eq!(result.status, RemoveStatus::Error); + assert_eq!(fs::read_to_string(&pkg).await.unwrap(), "not json!!!"); + } } diff --git a/tests/setup_matrix/run-case.sh b/tests/setup_matrix/run-case.sh index 328587be..09124e5e 100755 --- a/tests/setup_matrix/run-case.sh +++ b/tests/setup_matrix/run-case.sh @@ -77,12 +77,13 @@ log() { printf '[setup-matrix:%s] %s\n' "$SM_ID" "$*"; } json_str() { printf '%s' "$1" | tr -d '\r' | tr '\n' ' ' | sed 's/\\/\\\\/g; s/"/\\"/g'; } emit_result() { local actual="$1" primary_present="$2" setup_exit="$3" install_exit="$4" target="$5" status="$6" - printf '{"id":"%s","ecosystem":"%s","pm":"%s","scenario":"%s","patchset":"%s","run_setup":%s,"expect_applied":%s,"actual_applied":%s,"primary_marker_present":%s,"setup_exit":%s,"install_exit":%s,"target":"%s","status":"%s","notes":"%s"}\n' \ + printf '{"id":"%s","ecosystem":"%s","pm":"%s","scenario":"%s","patchset":"%s","run_setup":%s,"expect_applied":%s,"actual_applied":%s,"primary_marker_present":%s,"setup_exit":%s,"install_exit":%s,"check_after_setup_exit":%s,"remove_exit":%s,"check_after_remove_exit":%s,"remove_clean":%s,"target":"%s","status":"%s","notes":"%s"}\n' \ "$(json_str "$SM_ID")" "$(json_str "$SM_ECOSYSTEM")" "$(json_str "$SM_PM")" \ "$(json_str "$SM_SCENARIO")" "$(json_str "$SM_PATCHSET")" \ "$([ "$SM_RUN_SETUP" = 1 ] && echo true || echo false)" \ "$([ "$SM_EXPECT_APPLIED" = 1 ] && echo true || echo false)" \ "$actual" "$primary_present" "$setup_exit" "$install_exit" \ + "${CHECK_AFTER_SETUP_EXIT:-null}" "${REMOVE_EXIT:-null}" "${CHECK_AFTER_REMOVE_EXIT:-null}" "${REMOVE_CLEAN:-null}" \ "$(json_str "$target")" "$(json_str "$status")" "$(json_str "$NOTES")" >&3 } @@ -495,11 +496,18 @@ export SOCKET_TELEMETRY_DISABLED=1 SOCKET_EXPERIMENTAL_MAVEN=1 SOCKET_EXPERIMENT # 1. setup (configures hooks; no-op where there is no package.json) SETUP_EXIT="null" +CHECK_AFTER_SETUP_EXIT="null" if [ "$SM_RUN_SETUP" = 1 ]; then log "running: socket-patch setup --yes" "$SP_BIN" setup --yes --json; SETUP_EXIT=$? log "setup exit=$SETUP_EXIT" [ -f package.json ] && { log "package.json scripts after setup:"; grep -A6 '"scripts"' package.json || true; } + + # Read-only verification: a project we just configured must pass --check + # (exit 0). Recorded for the harness; does not touch disk. + log "running: socket-patch setup --check (after setup)" + "$SP_BIN" setup --check --json; CHECK_AFTER_SETUP_EXIT=$? + log "check-after-setup exit=$CHECK_AFTER_SETUP_EXIT" fi # 2. native install (this is where a configured hook fires) @@ -534,6 +542,30 @@ log "resolved target: ${TARGET:-} (candidates=$n_found)" [ "$n_found" -eq 0 ] && note "target file not found" log "marker '$check_marker' present: $APPLIED" +# 4. remove round-trip — only meaningful where setup configured hooks. +# Done LAST (after install + verify) so it cannot disturb the apply check. +# Asserts the inverse of setup: --remove strips the hook, --check then fails, +# and `socket-patch` no longer appears in the root package.json. +REMOVE_EXIT="null" +CHECK_AFTER_REMOVE_EXIT="null" +REMOVE_CLEAN="null" +if [ "$SM_RUN_SETUP" = 1 ] && [ -f package.json ]; then + log "running: socket-patch setup --remove --yes" + "$SP_BIN" setup --remove --yes --json; REMOVE_EXIT=$? + log "remove exit=$REMOVE_EXIT" + log "package.json scripts after remove:"; grep -A6 '"scripts"' package.json || true + + log "running: socket-patch setup --check (after remove)" + "$SP_BIN" setup --check --json; CHECK_AFTER_REMOVE_EXIT=$? + log "check-after-remove exit=$CHECK_AFTER_REMOVE_EXIT" + + if grep -q "socket-patch" package.json 2>/dev/null; then + REMOVE_CLEAN=false; note "remove left socket-patch in root package.json" + else + REMOVE_CLEAN=true + fi +fi + # Driver-level status: did actual match the aspirational expectation? want=$([ "$SM_EXPECT_APPLIED" = 1 ] && echo true || echo false) STATUS=fail From 29d65232db35e84a0e68b622ceafa8d087f889e7 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 2 Jun 2026 16:17:30 -0400 Subject: [PATCH 2/3] test(setup-matrix): verify check/remove behaviorally via install sequences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous round-trip inspected package.json (grepped for socket-patch) and flag state. Replace it with a behavioral verification driven by real (setup)·(package-manager install) cycles, for npm-family cases that run setup: install -> patch NOT applied (no hook yet) setup --check -> fails (not configured) setup --yes; setup --check -> passes (configured) reinstall -> patch applied (hook fires; the main actual_applied) setup --remove --yes; setup --check -> fails (reverted) reinstall -> patch NOT applied (hook gone) A clean `rm -rf node_modules` precedes each observation so the lifecycle hook acts on a pristine package. run-case.sh factors out do_install/verify_applied/ reset_modules; emit_result drops `remove_clean` and adds applied_before_setup, applied_after_remove, and check_before_setup_exit. The harness round_trip_failure asserts the patch bookends are not-applied and check goes false->true->false. Non-npm / no-setup cases keep the simple single-install flow unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/setup_matrix_common/mod.rs | 37 ++-- tests/setup_matrix/run-case.sh | 164 ++++++++++++------ 2 files changed, 133 insertions(+), 68 deletions(-) diff --git a/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs b/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs index d9b1c9b5..ed877cf2 100644 --- a/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs +++ b/crates/socket-patch-cli/tests/setup_matrix_common/mod.rs @@ -330,22 +330,40 @@ fn run_cases(label: &str, cases: Vec) { ); } -/// Validate the `setup --check` / `setup --remove` round-trip fields emitted by -/// the driver. Returns a failure message if the inverse of setup did not hold: -/// after configuring, `--check` must pass (0); `--remove` must succeed (0) and -/// strip socket-patch from package.json; and `--check` must then fail again -/// (non-zero). Returns `None` on success. +/// Validate the behavioral `(setup)·(install)` round-trip emitted by the driver. +/// Verifies — through real install cycles, not by reading package.json — that: +/// +/// 1. `setup --check` fails before setup, passes after setup, fails after +/// `setup --remove` (and remove itself succeeds); +/// 2. the patch is NOT applied before setup and NOT applied after remove +/// (the after-setup application is covered separately by the main +/// `actual_applied == expect_applied` assertion). +/// +/// Returns a failure message describing any violation, or `None` on success. fn round_trip_failure(case: &Case, res: &RunResult) -> Option { let parsed = res.parsed.as_ref()?; let int = |k: &str| parsed.get(k).and_then(|v| v.as_i64()); let boolean = |k: &str| parsed.get(k).and_then(|v| v.as_bool()); + let mut problems = Vec::new(); + + // (2) patch application bookends — only ever true while the hook is wired. + if boolean("applied_before_setup") == Some(true) { + problems.push("patch applied BEFORE setup (no hook should be configured yet)".to_string()); + } + if boolean("applied_after_remove") == Some(true) { + problems.push("patch still applied AFTER remove (hook should be gone)".to_string()); + } + + // (1) `setup --check` tracks the configured state: false → true → false. + let check_before = int("check_before_setup_exit"); let check_setup = int("check_after_setup_exit"); let remove = int("remove_exit"); let check_remove = int("check_after_remove_exit"); - let clean = boolean("remove_clean"); - let mut problems = Vec::new(); + if check_before == Some(0) { + problems.push("check-before-setup exit=0 (want non-zero; not configured yet)".to_string()); + } if check_setup != Some(0) { problems.push(format!("check-after-setup exit={check_setup:?} (want 0)")); } @@ -355,15 +373,12 @@ fn round_trip_failure(case: &Case, res: &RunResult) -> Option { if check_remove == Some(0) { problems.push("check-after-remove exit=0 (want non-zero; hook still present)".to_string()); } - if clean == Some(false) { - problems.push("socket-patch still present in package.json after remove".to_string()); - } if problems.is_empty() { return None; } Some(format!( - " - {}: check/remove round-trip failed [{}]\n{}", + " - {}: setup/install behavioral round-trip failed [{}]\n{}", case.id, problems.join("; "), indent(&res.raw) diff --git a/tests/setup_matrix/run-case.sh b/tests/setup_matrix/run-case.sh index 09124e5e..2e27ca52 100755 --- a/tests/setup_matrix/run-case.sh +++ b/tests/setup_matrix/run-case.sh @@ -77,13 +77,14 @@ log() { printf '[setup-matrix:%s] %s\n' "$SM_ID" "$*"; } json_str() { printf '%s' "$1" | tr -d '\r' | tr '\n' ' ' | sed 's/\\/\\\\/g; s/"/\\"/g'; } emit_result() { local actual="$1" primary_present="$2" setup_exit="$3" install_exit="$4" target="$5" status="$6" - printf '{"id":"%s","ecosystem":"%s","pm":"%s","scenario":"%s","patchset":"%s","run_setup":%s,"expect_applied":%s,"actual_applied":%s,"primary_marker_present":%s,"setup_exit":%s,"install_exit":%s,"check_after_setup_exit":%s,"remove_exit":%s,"check_after_remove_exit":%s,"remove_clean":%s,"target":"%s","status":"%s","notes":"%s"}\n' \ + printf '{"id":"%s","ecosystem":"%s","pm":"%s","scenario":"%s","patchset":"%s","run_setup":%s,"expect_applied":%s,"actual_applied":%s,"applied_before_setup":%s,"applied_after_remove":%s,"primary_marker_present":%s,"setup_exit":%s,"install_exit":%s,"check_before_setup_exit":%s,"check_after_setup_exit":%s,"remove_exit":%s,"check_after_remove_exit":%s,"target":"%s","status":"%s","notes":"%s"}\n' \ "$(json_str "$SM_ID")" "$(json_str "$SM_ECOSYSTEM")" "$(json_str "$SM_PM")" \ "$(json_str "$SM_SCENARIO")" "$(json_str "$SM_PATCHSET")" \ "$([ "$SM_RUN_SETUP" = 1 ] && echo true || echo false)" \ "$([ "$SM_EXPECT_APPLIED" = 1 ] && echo true || echo false)" \ - "$actual" "$primary_present" "$setup_exit" "$install_exit" \ - "${CHECK_AFTER_SETUP_EXIT:-null}" "${REMOVE_EXIT:-null}" "${CHECK_AFTER_REMOVE_EXIT:-null}" "${REMOVE_CLEAN:-null}" \ + "$actual" "${APPLIED_BEFORE_SETUP:-null}" "${APPLIED_AFTER_REMOVE:-null}" "$primary_present" \ + "$setup_exit" "$install_exit" \ + "${CHECK_BEFORE_SETUP_EXIT:-null}" "${CHECK_AFTER_SETUP_EXIT:-null}" "${REMOVE_EXIT:-null}" "${CHECK_AFTER_REMOVE_EXIT:-null}" \ "$(json_str "$target")" "$(json_str "$status")" "$(json_str "$NOTES")" >&3 } @@ -449,6 +450,49 @@ resolve_targets() { esac } +# --- native install dispatch (layout-aware) -------------------------- +do_install() { + case "$SM_LAYOUT" in + workspace) run_install_workspace ;; + monorepo) run_install_monorepo ;; + *) run_install ;; + esac +} + +# Wipe installed modules so the NEXT install re-fetches a pristine copy and +# re-fires the lifecycle hook. This is what lets us observe the patch-apply +# BEHAVIOR (marker present/absent on a freshly installed file) at each stage +# of the (setup)·(install) sequence, rather than inspecting package.json. +reset_modules() { + rm -rf node_modules packages/*/node_modules 2>/dev/null || true +} + +# Resolve every on-disk copy of the patched file and decide whether the patch +# was applied (marker present). Sets APPLIED / PRIMARY_PRESENT / TARGET. +verify_applied() { + local check_marker="$SM_MARKER" + [ "$SM_PATCHSET" = alt ] && check_marker="$SM_ALT_MARKER" + APPLIED=false + PRIMARY_PRESENT=null + TARGET="" + local n_found=0 cand + while IFS= read -r cand; do + [ -n "$cand" ] && [ -f "$cand" ] || continue + n_found=$((n_found + 1)) + [ -z "$TARGET" ] && TARGET="$cand" + if grep -q "$check_marker" "$cand" 2>/dev/null; then APPLIED=true; TARGET="$cand"; fi + if grep -q "$SM_MARKER" "$cand" 2>/dev/null; then PRIMARY_PRESENT=true; fi + done < <(resolve_targets) + [ "$PRIMARY_PRESENT" = null ] && [ "$n_found" -gt 0 ] && PRIMARY_PRESENT=false + log "verify: marker '$check_marker' present=$APPLIED (candidates=$n_found, target=${TARGET:-})" +} + +# npm-family is the surface `setup` actually configures today — the only place +# the behavioral check/remove round-trip is expected to do real work. +is_npm_family() { + [[ "$SM_PM" =~ ^(npm|yarn|pnpm|bun)$ ]] || [ "$SM_LAYOUT" = monorepo ] +} + # ============================ main ==================================== log "binary: $SP_BIN ($("$SP_BIN" --version 2>/dev/null || echo '??')) layout=$SM_LAYOUT" @@ -494,76 +538,82 @@ export SOCKET_TELEMETRY_DISABLED=1 SOCKET_EXPERIMENTAL_MAVEN=1 SOCKET_EXPERIMENT # make every member apply target the root manifest and fail with "no # packages found on disk" mid-install, breaking `npm install`. -# 1. setup (configures hooks; no-op where there is no package.json) +# 1-3. Configure + install + verify. +# +# For npm-family cases that run setup we exercise the FULL behavioral sequence +# — (install)·(setup)·(install)·(remove)·(install) — observing both the patch +# marker and `setup --check` at each stage. A clean reinstall precedes every +# observation so the lifecycle hook acts on a pristine package. This verifies +# behavior end-to-end rather than reading package.json: +# * patch: NOT applied before setup → applied after setup → NOT applied after remove +# * check: fails before setup → passes after setup → fails after remove +# +# Every other case (run_setup=0, or non-npm-family ecosystems) keeps the simple +# single-install flow, preserving the existing aspirational expect_applied +# classification untouched. SETUP_EXIT="null" +CHECK_BEFORE_SETUP_EXIT="null" CHECK_AFTER_SETUP_EXIT="null" -if [ "$SM_RUN_SETUP" = 1 ]; then +REMOVE_EXIT="null" +CHECK_AFTER_REMOVE_EXIT="null" +APPLIED_BEFORE_SETUP=null +APPLIED_AFTER_REMOVE=null +INSTALL_EXIT="null" + +if is_npm_family && [ "$SM_RUN_SETUP" = 1 ]; then + # (1) BEFORE setup: no hook configured → install must NOT apply the patch. + log "[before-setup] install for pm=$SM_PM (layout=$SM_LAYOUT)" + do_install; log "[before-setup] install exit=$?" + verify_applied; APPLIED_BEFORE_SETUP="$APPLIED" + + # (2) check must report "needs configuration" (non-zero) before setup. + "$SP_BIN" setup --check --json; CHECK_BEFORE_SETUP_EXIT=$? + log "check-before-setup exit=$CHECK_BEFORE_SETUP_EXIT" + + # (3) setup, then check must report "configured" (zero). log "running: socket-patch setup --yes" "$SP_BIN" setup --yes --json; SETUP_EXIT=$? log "setup exit=$SETUP_EXIT" [ -f package.json ] && { log "package.json scripts after setup:"; grep -A6 '"scripts"' package.json || true; } - - # Read-only verification: a project we just configured must pass --check - # (exit 0). Recorded for the harness; does not touch disk. - log "running: socket-patch setup --check (after setup)" "$SP_BIN" setup --check --json; CHECK_AFTER_SETUP_EXIT=$? log "check-after-setup exit=$CHECK_AFTER_SETUP_EXIT" -fi -# 2. native install (this is where a configured hook fires) -log "running install for pm=$SM_PM (layout=$SM_LAYOUT)" -case "$SM_LAYOUT" in - workspace) run_install_workspace ;; - monorepo) run_install_monorepo ;; - *) run_install ;; -esac -INSTALL_EXIT=$? -log "install exit=$INSTALL_EXIT" - -# 3. verify — applied if ANY discovered copy of the patched file carries -# the expected marker (covers hoisting, the pnpm store, member dirs and -# the shared venv in workspace/monorepo layouts). -check_marker="$SM_MARKER" -[ "$SM_PATCHSET" = alt ] && check_marker="$SM_ALT_MARKER" -APPLIED=false -PRIMARY_PRESENT=null -TARGET="" -n_found=0 -while IFS= read -r cand; do - [ -n "$cand" ] && [ -f "$cand" ] || continue - n_found=$((n_found + 1)) - [ -z "$TARGET" ] && TARGET="$cand" - if grep -q "$check_marker" "$cand" 2>/dev/null; then APPLIED=true; TARGET="$cand"; fi - if grep -q "$SM_MARKER" "$cand" 2>/dev/null; then PRIMARY_PRESENT=true; fi -done < <(resolve_targets) -[ "$PRIMARY_PRESENT" = null ] && [ "$n_found" -gt 0 ] && PRIMARY_PRESENT=false -note "candidate files found: $n_found" -log "resolved target: ${TARGET:-} (candidates=$n_found)" -[ "$n_found" -eq 0 ] && note "target file not found" -log "marker '$check_marker' present: $APPLIED" - -# 4. remove round-trip — only meaningful where setup configured hooks. -# Done LAST (after install + verify) so it cannot disturb the apply check. -# Asserts the inverse of setup: --remove strips the hook, --check then fails, -# and `socket-patch` no longer appears in the root package.json. -REMOVE_EXIT="null" -CHECK_AFTER_REMOVE_EXIT="null" -REMOVE_CLEAN="null" -if [ "$SM_RUN_SETUP" = 1 ] && [ -f package.json ]; then + # (4) AFTER setup: clean reinstall → the hook fires → MAIN applied result. + reset_modules + log "[after-setup] install for pm=$SM_PM (layout=$SM_LAYOUT)" + do_install; INSTALL_EXIT=$? + log "[after-setup] install exit=$INSTALL_EXIT" + verify_applied # sets the canonical APPLIED / PRIMARY_PRESENT / TARGET + + # (5) remove, then check must report "needs configuration" (non-zero) again. log "running: socket-patch setup --remove --yes" "$SP_BIN" setup --remove --yes --json; REMOVE_EXIT=$? log "remove exit=$REMOVE_EXIT" - log "package.json scripts after remove:"; grep -A6 '"scripts"' package.json || true - - log "running: socket-patch setup --check (after remove)" + [ -f package.json ] && { log "package.json scripts after remove:"; grep -A6 '"scripts"' package.json || true; } "$SP_BIN" setup --check --json; CHECK_AFTER_REMOVE_EXIT=$? log "check-after-remove exit=$CHECK_AFTER_REMOVE_EXIT" - if grep -q "socket-patch" package.json 2>/dev/null; then - REMOVE_CLEAN=false; note "remove left socket-patch in root package.json" - else - REMOVE_CLEAN=true + # (6) AFTER remove: clean reinstall → no hook → must NOT apply the patch. + # Preserve the main (after-setup) result while re-probing the disk. + _MAIN_APPLIED="$APPLIED"; _MAIN_PRIMARY="$PRIMARY_PRESENT"; _MAIN_TARGET="$TARGET" + reset_modules + log "[after-remove] install for pm=$SM_PM (layout=$SM_LAYOUT)" + do_install; log "[after-remove] install exit=$?" + verify_applied; APPLIED_AFTER_REMOVE="$APPLIED" + APPLIED="$_MAIN_APPLIED"; PRIMARY_PRESENT="$_MAIN_PRIMARY"; TARGET="$_MAIN_TARGET" +else + # Simple flow: optional setup (no-op where there is no package.json), one + # install, one verify. + if [ "$SM_RUN_SETUP" = 1 ]; then + log "running: socket-patch setup --yes" + "$SP_BIN" setup --yes --json; SETUP_EXIT=$? + log "setup exit=$SETUP_EXIT" + [ -f package.json ] && { log "package.json scripts after setup:"; grep -A6 '"scripts"' package.json || true; } fi + log "running install for pm=$SM_PM (layout=$SM_LAYOUT)" + do_install; INSTALL_EXIT=$? + log "install exit=$INSTALL_EXIT" + verify_applied fi # Driver-level status: did actual match the aspirational expectation? From b519a75b23823c08b2284fe02a189e4e1715e020 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 2 Jun 2026 16:26:59 -0400 Subject: [PATCH 3/3] test(setup-matrix): verify patches by RUNNING the code, not grepping the file The matrix decided "applied" by scanning the patched file for the marker string. Now it actually EXECUTES the patched module with the ecosystem's standard runner and checks for the marker in the runtime output: npm/yarn/pnpm -> node bun -> bun deno -> deno run pip -> ./venv/bin/python uv -> uv run python poetry/pdm/hatch -> *run python To make the patched code observable at runtime, the committed blob is now runnable: `console.log("MARKER")` for JS, `print("MARKER")` for Python. Compiled/loaded ecosystems we can't execute (cargo/go/maven/nuget/gem/ composer) keep an inert comment and fall back to reading the file (`cat`), preserving today's behavior for those gaps. verify_applied() runs every resolved copy of the file (covers hoisting, the pnpm store, and workspace/monorepo member dirs) and ORs the results. Also fixes a parallel-test race: the blob was written to a fixed /tmp/sm_blob, so concurrent package-manager test fns clobbered each other's fixture (afterHash mismatch -> apply no-op). Each case now uses its own mktemp file. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/setup_matrix/run-case.sh | 74 ++++++++++++++++++++++++++++------ 1 file changed, 62 insertions(+), 12 deletions(-) diff --git a/tests/setup_matrix/run-case.sh b/tests/setup_matrix/run-case.sh index 2e27ca52..418c48e3 100755 --- a/tests/setup_matrix/run-case.sh +++ b/tests/setup_matrix/run-case.sh @@ -136,6 +136,20 @@ SHIM } # --- committed patch fixture ------------------------------------------ +# The patched blob is RUNNABLE code that emits the marker on stdout when the +# file is executed, so verification can RUN the patched module with the +# ecosystem's standard runner (node/bun/python) and observe the marker at +# runtime — not merely scan the file for the string. Compiled/loaded +# ecosystems we can't execute keep an inert comment (verified by reading the +# file; see `run_file`). +marker_blob() { # $1 = marker -> runnable payload on stdout + case "$SM_ECOSYSTEM" in + npm|deno|monorepo) printf 'console.log("%s");\n' "$1" ;; + pypi) printf 'print("%s")\n' "$1" ;; + *) printf '/* %s */\n' "$1" ;; + esac +} + write_manifest() { # $1=purl $2=key $3=afterHash cat > .socket/manifest.json < .socket/manifest.json note "empty manifest" ;; wrong) # A patch for a package that is NOT installed: nothing should match. - local body="/* $SM_MARKER */"; printf '%s\n' "$body" > /tmp/sm_blob - local h; h="$(git_sha256 /tmp/sm_blob)"; cp /tmp/sm_blob ".socket/blobs/$h" + marker_blob "$SM_MARKER" > "$blob_tmp" + local h; h="$(git_sha256 "$blob_tmp")"; cp "$blob_tmp" ".socket/blobs/$h" write_manifest "$WRONG_PURL" "$SM_MANIFEST_KEY" "$h" note "manifest targets absent purl $WRONG_PURL" ;; alt) - local body="/* $SM_ALT_MARKER */"; printf '%s\n' "$body" > /tmp/sm_blob - local h; h="$(git_sha256 /tmp/sm_blob)"; cp /tmp/sm_blob ".socket/blobs/$h" + marker_blob "$SM_ALT_MARKER" > "$blob_tmp" + local h; h="$(git_sha256 "$blob_tmp")"; cp "$blob_tmp" ".socket/blobs/$h" write_manifest "$SM_PURL" "$SM_MANIFEST_KEY" "$h" ;; *) # primary - local body="/* $SM_MARKER */"; printf '%s\n' "$body" > /tmp/sm_blob - local h; h="$(git_sha256 /tmp/sm_blob)"; cp /tmp/sm_blob ".socket/blobs/$h" + marker_blob "$SM_MARKER" > "$blob_tmp" + local h; h="$(git_sha256 "$blob_tmp")"; cp "$blob_tmp" ".socket/blobs/$h" write_manifest "$SM_PURL" "$SM_MANIFEST_KEY" "$h" ;; esac + rm -f "$blob_tmp" } # --- per-PM project scaffold (must exist before setup runs) ----------- @@ -467,24 +487,54 @@ reset_modules() { rm -rf node_modules packages/*/node_modules 2>/dev/null || true } -# Resolve every on-disk copy of the patched file and decide whether the patch -# was applied (marker present). Sets APPLIED / PRIMARY_PRESENT / TARGET. +# Execute a single patched file with the ecosystem's STANDARD runner so the +# patched code actually runs; its stdout/stderr (where the marker would be +# printed) is emitted for the caller to inspect. npm→node, bun→bun, deno→deno, +# pip→the venv's python3, uv→uv run, poetry/pdm/hatch→their `run`. For +# compiled/loaded ecosystems we cannot execute (cargo/go/maven/nuget/gem/ +# composer) we `cat` the file so its inert marker comment is still observed — +# matching the previous file-based behavior for those gaps. +run_file() { # $1 = absolute path to the resolved package file + case "$SM_ECOSYSTEM" in + npm|monorepo) + case "$SM_PM" in + bun) bun "$1" ;; + *) node "$1" ;; + esac ;; + deno) deno run -A "$1" ;; + pypi) + case "$SM_PM" in + uv) uv run python "$1" ;; + poetry) poetry run python "$1" ;; + pdm) pdm run python "$1" ;; + hatch) hatch run python "$1" ;; + pip) ./venv/bin/python "$1" ;; + *) python3 "$1" ;; + esac ;; + *) cat "$1" ;; + esac +} + +# Decide whether the patch was applied by RUNNING every on-disk copy of the +# patched file and checking whether the marker appears in its runtime output. +# Sets APPLIED / PRIMARY_PRESENT / TARGET. verify_applied() { local check_marker="$SM_MARKER" [ "$SM_PATCHSET" = alt ] && check_marker="$SM_ALT_MARKER" APPLIED=false PRIMARY_PRESENT=null TARGET="" - local n_found=0 cand + local n_found=0 cand out while IFS= read -r cand; do [ -n "$cand" ] && [ -f "$cand" ] || continue n_found=$((n_found + 1)) [ -z "$TARGET" ] && TARGET="$cand" - if grep -q "$check_marker" "$cand" 2>/dev/null; then APPLIED=true; TARGET="$cand"; fi - if grep -q "$SM_MARKER" "$cand" 2>/dev/null; then PRIMARY_PRESENT=true; fi + out="$(run_file "$cand" 2>&1)" + if printf '%s' "$out" | grep -q "$check_marker"; then APPLIED=true; TARGET="$cand"; fi + if printf '%s' "$out" | grep -q "$SM_MARKER"; then PRIMARY_PRESENT=true; fi done < <(resolve_targets) [ "$PRIMARY_PRESENT" = null ] && [ "$n_found" -gt 0 ] && PRIMARY_PRESENT=false - log "verify: marker '$check_marker' present=$APPLIED (candidates=$n_found, target=${TARGET:-})" + log "verify(run): marker '$check_marker' in runtime output=$APPLIED (candidates=$n_found, target=${TARGET:-})" } # npm-family is the surface `setup` actually configures today — the only place