From c713749bcfd8c6f08295d71f5f5aaadb0772d5dd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 08:24:41 +0000 Subject: [PATCH 1/5] Start fix for #998, #1063, #1123 Assisted-by: Claude Code:claude-opus-5-5 From 2123045da499ef34850ea886e43467f062f095a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 08:47:07 +0000 Subject: [PATCH 2/5] Test manifest stat errors and path spellings Adds regression tests: .socket as a plain file and a self-referencing manifest symlink must fail every command with manifest_unreadable (#998), and a directory at the manifest path must get the same verdict whether the path is the default, spelled with --manifest-path (relative or absolute) or set through SOCKET_MANIFEST_PATH (#1123). Adds rollback to the manifest load error matrix and a core manifest_exists probe that treats only NotFound as absence. Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/manifest_load_error_matrix.rs | 101 ++++++++++++++++-- .../src/manifest/operations.rs | 53 +++++++++ 2 files changed, 147 insertions(+), 7 deletions(-) diff --git a/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs b/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs index 79f48b63f..44a47bbbc 100644 --- a/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs +++ b/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs @@ -7,6 +7,11 @@ //! different codes (`apply_failed`, `repair_failed`, `invalid_manifest`, //! `manifest_unreadable`, `manifest_invalid`). Nothing here reaches the //! network. +//! +//! #998: the commands that probe for a manifest before loading it must +//! read a stat error (`.socket` as a plain file, a symlink loop) as +//! unreadable, not as "no manifest". #1123: the verdict must not depend on +//! how the manifest path is spelled. use std::path::Path; @@ -15,13 +20,14 @@ use socket_patch_cli::args::{GLOBAL_ARG_ENV_VARS, LOCAL_ARG_ENV_VARS}; #[path = "common/hermetic.rs"] mod hermetic; -fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { +fn run(cwd: &Path, args: &[&str], env: &[(&str, &str)]) -> (i32, String, String) { let mut cmd = hermetic::binary_command(); hermetic::scrub_extra(&mut cmd, &[hermetic::Extra::Venv]); cmd.args(args).current_dir(cwd); for var in GLOBAL_ARG_ENV_VARS.iter().chain(LOCAL_ARG_ENV_VARS.iter()) { cmd.env_remove(var); } + cmd.envs(env.iter().copied()); cmd.env("SOCKET_TELEMETRY_DISABLED", "1"); // Unroutable: a case that reached the network would fail differently. cmd.env("SOCKET_API_URL", "http://127.0.0.1:1"); @@ -48,6 +54,7 @@ const MATRIX: &[(&[&str], &str, i32)] = &[ (&["apply", "--json", "--offline"], "apply", 1), (&["apply", "--check", "--json"], "apply", 1), (&["repair", "--json", "--offline"], "repair", 1), + (&["rollback", "--json", "--offline"], "rollback", 1), (&["vendor", "--json", "--offline"], "vendor", 1), (&["vendor", "--check", "--json"], "vendor", 1), (&["vex", "--json", "--output", "out.json"], "vex", 2), @@ -55,25 +62,62 @@ const MATRIX: &[(&[&str], &str, i32)] = &[ /// A project holding `package.json` plus whatever `seed` puts at /// `.socket/manifest.json`. -fn project(seed: impl Fn(&Path)) -> tempfile::TempDir { +fn project(seed: &dyn Fn(&Path)) -> tempfile::TempDir { + project_root(&|root| { + let socket = root.join(".socket"); + std::fs::create_dir_all(&socket).unwrap(); + seed(&socket.join("manifest.json")); + }) +} + +/// A project holding `package.json` plus whatever `seed` puts in its root. +fn project_root(seed: &dyn Fn(&Path)) -> tempfile::TempDir { let tmp = tempfile::tempdir().unwrap(); std::fs::write( tmp.path().join("package.json"), r#"{"name":"proj","version":"1.0.0"}"#, ) .unwrap(); - let socket = tmp.path().join(".socket"); - std::fs::create_dir_all(&socket).unwrap(); - seed(&socket.join("manifest.json")); + seed(tmp.path()); tmp } +/// How a run names the manifest (#1123): the default, or spelled out +/// through the flag (relative or absolute) or the environment. +#[derive(Clone, Copy, Debug)] +enum Spelling { + Default, + FlagRelative, + FlagAbsolute, + Env, +} + fn assert_matrix(seed: impl Fn(&Path), code: &str) { + assert_matrix_in(project, &seed, code, Spelling::Default); +} + +fn assert_matrix_in( + make: fn(&dyn Fn(&Path)) -> tempfile::TempDir, + seed: &dyn Fn(&Path), + code: &str, + spelling: Spelling, +) { for (argv, command, exit) in MATRIX { // A fresh project per command: nothing one run leaves behind (an // apply.lock, a VEX file) can steer the next. - let tmp = project(&seed); - let (got_exit, stdout, stderr) = run(tmp.path(), argv); + let tmp = make(seed); + let absolute = tmp.path().join(".socket").join("manifest.json"); + let absolute = absolute.to_str().unwrap(); + let mut args: Vec<&str> = argv.to_vec(); + let mut env: Vec<(&str, &str)> = Vec::new(); + match spelling { + Spelling::Default => {} + Spelling::FlagRelative => args.extend(["--manifest-path", "./.socket/manifest.json"]), + Spelling::FlagAbsolute => args.extend(["--manifest-path", absolute]), + Spelling::Env => env.push(("SOCKET_MANIFEST_PATH", "./.socket/manifest.json")), + } + let argv = (&args, spelling); + let (got_exit, stdout, stderr) = run(tmp.path(), &args, &env); let v: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap_or_else(|e| { panic!("{argv:?}: stdout must be one JSON envelope ({e}): {stdout:?}\nstderr={stderr}") }); @@ -112,3 +156,46 @@ fn unreadable_manifest_is_manifest_unreadable_on_every_command() { "manifest_unreadable", ); } + +/// #998: `.socket` is a plain file, so stating the manifest fails with +/// ENOTDIR. That is unreadable, not missing: `apply` and `vendor` used to +/// report `noManifest` and exit 0, the others `manifest_not_found`. Runs +/// as root, unlike a `chmod 000` fixture. +#[test] +fn socket_dir_as_file_is_manifest_unreadable_on_every_command() { + assert_matrix_in( + project_root, + &|root| std::fs::write(root.join(".socket"), "not a directory").unwrap(), + "manifest_unreadable", + Spelling::Default, + ); +} + +/// #998: a manifest symlinked to itself fails stat with ELOOP. +#[cfg(unix)] +#[test] +fn manifest_symlink_loop_is_manifest_unreadable_on_every_command() { + assert_matrix( + |path| std::os::unix::fs::symlink("manifest.json", path).unwrap(), + "manifest_unreadable", + ); +} + +/// #1123: a directory at the manifest path gets the same verdict however +/// the path is spelled. A spelled-out path used to exit 2 from argument +/// validation with no `--json` document. +#[test] +fn directory_manifest_verdict_does_not_depend_on_spelling() { + for spelling in [ + Spelling::FlagRelative, + Spelling::FlagAbsolute, + Spelling::Env, + ] { + assert_matrix_in( + project, + &|path| std::fs::create_dir_all(path).unwrap(), + "manifest_unreadable", + spelling, + ); + } +} diff --git a/crates/socket-patch-core/src/manifest/operations.rs b/crates/socket-patch-core/src/manifest/operations.rs index 4e00a7ac9..c98b6b50f 100644 --- a/crates/socket-patch-core/src/manifest/operations.rs +++ b/crates/socket-patch-core/src/manifest/operations.rs @@ -68,6 +68,20 @@ pub async fn read_manifest( parse_manifest(json).map(Some) } +/// Whether a manifest exists at `path`, by [`read_manifest`]'s rule: only +/// `NotFound` is absence. Any other stat error (an unsearchable `.socket/`, +/// `.socket` as a plain file, a symlink loop) is returned, so a caller's +/// "no manifest" path never runs for a manifest it merely cannot read. +/// Something that is not a regular file still counts as present: +/// [`read_manifest`] then refuses it, whatever the path's spelling. +pub async fn manifest_exists(path: impl AsRef) -> Result { + match tokio::fs::metadata(path.as_ref()).await { + Ok(_) => Ok(true), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(e) => Err(e), + } +} + /// Write a manifest to the filesystem with pretty-printed JSON. /// /// The write is atomic: the JSON is staged in a sibling temp file, fsync'd, @@ -471,6 +485,45 @@ mod tests { ); } + // #998: `manifest_exists` keeps `read_manifest`'s rule. Only a missing + // file is absent; a stat error is returned, never read as "no manifest". + #[tokio::test] + async fn test_manifest_exists_only_not_found_is_absent() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("manifest.json"); + assert!(!manifest_exists(&path).await.unwrap()); + + write_manifest(&path, &create_test_manifest()) + .await + .unwrap(); + assert!(manifest_exists(&path).await.unwrap()); + + // A directory is present; `read_manifest` refuses it. + let as_dir = dir.path().join("dir").join("manifest.json"); + std::fs::create_dir_all(&as_dir).unwrap(); + assert!(manifest_exists(&as_dir).await.unwrap()); + assert!(read_manifest(&as_dir).await.is_err()); + + // `.socket` as a plain file: ENOTDIR, not NotFound. + let socket_file = dir.path().join("socket-file"); + std::fs::write(&socket_file, "x").unwrap(); + let under_file = socket_file.join("manifest.json"); + let err = manifest_exists(&under_file).await.unwrap_err(); + assert_ne!(err.kind(), std::io::ErrorKind::NotFound); + assert!(read_manifest(&under_file).await.is_err()); + } + + #[cfg(unix)] + #[tokio::test] + async fn test_manifest_exists_symlink_loop_is_error() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("manifest.json"); + std::os::unix::fs::symlink("manifest.json", &path).unwrap(); + let err = manifest_exists(&path).await.unwrap_err(); + assert_ne!(err.kind(), std::io::ErrorKind::NotFound); + assert!(read_manifest(&path).await.is_err()); + } + // Regression: write_manifest -> read_manifest must preserve the full record, // not merely the patch count. Guards against a serializer that drops nested // fields (file hashes, vulnerabilities) while still round-tripping the keys. From a4395ceb66b4e1c472a9b8f2c92e15188176d2d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 08:53:22 +0000 Subject: [PATCH 3/5] Fail closed when the manifest can't be read apply, apply --check and vendor treated any error stating .socket/manifest.json as "no manifest" and exited 0 with noManifest, so an unsearchable .socket/, .socket checked in as a file or a symlink loop silently skipped every patch. repair, remove and rollback said manifest_not_found. All six now ask core's manifest_exists, which keeps read_manifest's rule (only NotFound is absence), and refuse with manifest_unreadable before taking a lock (#998). scan's GC passes read an unloadable manifest as missing too. They now report the manifest_invalid / manifest_unreadable warning instead of a clean summary (#1063). A --manifest-path naming a directory exited 2 with no --json output when the path was spelled out, but 1 with manifest_unreadable under the default spelling. Argument validation no longer judges it, so every spelling gets manifest_unreadable (#1123). Un-ignores apply_with_unreadable_socket_dir_fails_closed. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/src/args.rs | 21 ++--- crates/socket-patch-cli/src/commands/apply.rs | 27 +++++- .../socket-patch-cli/src/commands/remove.rs | 18 +++- .../socket-patch-cli/src/commands/repair.rs | 24 ++++- .../socket-patch-cli/src/commands/rollback.rs | 19 +++- .../socket-patch-cli/src/commands/scan/gc.rs | 92 +++++++++++++++++-- .../socket-patch-cli/src/commands/vendor.rs | 22 ++++- .../tests/apply/apply_invariants.rs | 4 - .../socket-patch-cli/tests/cli_parse_list.rs | 24 +++-- .../tests/cli_path_flags_validated.rs | 20 ++-- 10 files changed, 216 insertions(+), 55 deletions(-) diff --git a/crates/socket-patch-cli/src/args.rs b/crates/socket-patch-cli/src/args.rs index 2a8b5d6f6..5321d118b 100644 --- a/crates/socket-patch-cli/src/args.rs +++ b/crates/socket-patch-cli/src/args.rs @@ -357,8 +357,8 @@ pub struct GlobalArgs { impl GlobalArgs { /// Reject path flags that name nothing: `--cwd` and `--global-prefix` /// (flag or env) must be existing directories, and a `--manifest-path` - /// other than the default must sit in an existing project directory and - /// must not itself be a directory. `main` maps `Err` to the usage exit + /// other than the default must sit in an existing project directory. + /// `main` maps `Err` to the usage exit /// (2), the same exit a hosted/vendored `scan` PATH that is not a /// directory gets. /// @@ -399,16 +399,15 @@ impl GlobalArgs { )); } } + // A manifest path that names a directory is not refused here: that + // is a corrupt project, not a bad flag, and `read_manifest` refuses + // it as `manifest_unreadable` under every spelling of the path. A + // check keyed on the flag's text gave the explicit spellings exit 2 + // with no `--json` document instead (#1123). if self.manifest_path != DEFAULT_PATCH_MANIFEST_PATH { - let manifest = self.resolved_manifest_path(); - if manifest.is_dir() { - return Err(not_dir( - "--manifest-path", - "SOCKET_MANIFEST_PATH", - &manifest, - "is a directory, not a manifest file", - )); - } + // `.`-free for the message: `--cwd .` joined onto + // `./.socket/manifest.json` would print `././.socket/…`. + let manifest: PathBuf = self.resolved_manifest_path().components().collect(); // The directory that must exist: the project of a `.socket/` // manifest (`.socket/` itself is created on demand), else the // manifest file's own directory. diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index 428ec63cb..c4c098be3 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -6,7 +6,7 @@ use socket_patch_core::crawlers::ruby_crawler::{ use socket_patch_core::crawlers::{ bun_uses_global_store, detect_npm_pkg_manager, Ecosystem, NpmPkgManager, RubyCrawler, }; -use socket_patch_core::manifest::operations::read_manifest; +use socket_patch_core::manifest::operations::{manifest_exists, read_manifest}; use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord}; use socket_patch_core::patch::apply::{ apply_package_patch, verify_file_patch, ApplyResult, MismatchPolicy, PatchSources, VerifyStatus, @@ -868,6 +868,22 @@ fn manifest_targets_npm(manifest: &PatchManifest) -> bool { /// The yarn-berry PnP refusal's envelope error text. const YARN_PNP_UNSUPPORTED: &str = "yarn-berry Plug'n'Play layout is not supported by socket-patch (packages live inside .yarn/cache zips). Use `yarn patch ` instead."; +/// A manifest that exists but cannot be stated, refused before the API +/// client, the lock or telemetry are touched: the same envelope and exit +/// as the load failure further down (#931, #998). +fn refuse_unreadable_manifest(args: &ApplyArgs, manifest_path: &Path, e: &std::io::Error) -> i32 { + let err = crate::json_envelope::manifest_load_error(manifest_path, e); + if args.common.json { + let mut env = Envelope::new(Command::Apply); + env.dry_run = args.common.dry_run; + env.mark_error(err); + println!("{}", env.to_pretty_json()); + } else { + eprintln!("Error: {}", crate::ui::sentence_case(&err.message)); + } + 1 +} + fn refuse_yarn_pnp(args: &ApplyArgs) -> i32 { refuse_yarn_pnp_with_skips(args, &[]) } @@ -921,7 +937,14 @@ pub async fn run(args: ApplyArgs) -> i32 { // every install). // Nothing below this gate is touched — no API client (its config read, // stderr advisory and org-slug round-trip), no lock, no `.socket/`. - if tokio::fs::metadata(&manifest_path).await.is_err() { + // Only a missing file is "no manifest": one apply cannot even stat + // (an unsearchable `.socket/`, `.socket` as a file, a symlink loop) + // fails closed, or every patch would be skipped with exit 0 (#998). + let manifest_present = match manifest_exists(&manifest_path).await { + Ok(present) => present, + Err(e) => return refuse_unreadable_manifest(&args, &manifest_path, &e), + }; + if !manifest_present { // A yarn-PnP layout refuses loudly even with no manifest: scan // cannot discover PnP packages (they live inside .yarn/cache zips), // so it never writes one — without this hoisted check the layout diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index 3965a9ee2..16a70cbcb 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -5,7 +5,7 @@ use socket_patch_core::ledgers::hosted_pins_matching; use socket_patch_core::manifest::cleanup_blobs::{ format_bytes, format_cleanup_result_for, ArtifactReferences, }; -use socket_patch_core::manifest::operations::{read_manifest, write_manifest}; +use socket_patch_core::manifest::operations::{manifest_exists, read_manifest, write_manifest}; use socket_patch_core::manifest::schema::PatchManifest; use socket_patch_core::patch::redirect::upstream::HostedPin; use socket_patch_core::telemetry::{track_patch_remove_failed, track_patch_removed, TelemetryAuth}; @@ -365,7 +365,21 @@ pub async fn run(args: RemoveArgs) -> i32 { // remove restores the global copies and drops their manifest records // only. let project_state = crate::commands::project_state_in_scope(&args.common); - let manifest_missing = tokio::fs::metadata(&manifest_path).await.is_err(); + // Only a missing file is "no manifest"; one remove cannot even stat is + // unreadable, not a bare project (#998). + let manifest_missing = match manifest_exists(&manifest_path).await { + Ok(present) => !present, + Err(e) => { + let err = crate::json_envelope::manifest_load_error(&manifest_path, &e); + emit_error_envelope( + args.common.json, + args.common.dry_run, + &err.code, + err.message, + ); + return 1; + } + }; let hosted_inventory = if project_state { crate::commands::hosted_inventory(&args.common, &args.common.project_root()).await } else { diff --git a/crates/socket-patch-cli/src/commands/repair.rs b/crates/socket-patch-cli/src/commands/repair.rs index 57dbf0ddd..bd9436638 100644 --- a/crates/socket-patch-cli/src/commands/repair.rs +++ b/crates/socket-patch-cli/src/commands/repair.rs @@ -7,7 +7,7 @@ use socket_patch_core::api::client::{get_api_client_with_overrides, ApiClient}; use socket_patch_core::manifest::cleanup_blobs::{ format_all_in_use, format_cleanup_result_for, ArtifactReferences, CleanupResult, }; -use socket_patch_core::manifest::operations::read_manifest; +use socket_patch_core::manifest::operations::{manifest_exists, read_manifest}; use socket_patch_core::telemetry::{ track_patch_repair_failed, track_patch_repaired, TelemetryAuth, }; @@ -65,7 +65,27 @@ pub async fn run(args: RepairArgs) -> i32 { // for a ledger-less project, and that result is reused under the lock. let mut vendor_references: Option> = None; - if tokio::fs::metadata(&manifest_path).await.is_err() { + // Only a missing file is "no manifest"; one repair cannot even stat is + // unreadable, not a project with nothing to repair (#998). + let manifest_present = match manifest_exists(&manifest_path).await { + Ok(present) => present, + Err(e) => { + let err = crate::json_envelope::manifest_load_error(&manifest_path, &e); + if args.common.json { + let env = error_envelope( + Command::Repair, + args.common.dry_run, + &err.code, + &err.message, + ); + println!("{}", env.to_pretty_json()); + } else { + eprintln!("Error: {}", crate::ui::sentence_case(&err.message)); + } + return 1; + } + }; + if !manifest_present { // Hosted mode leaves no local artifacts to repair: the lockfiles // point at patch.socket.dev URLs, not `.socket/vendor/...`, and // there is no manifest or vendor ledger. A project whose only trace diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 6f98e9957..fdda420c6 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -4,7 +4,7 @@ use socket_patch_core::api::client::{get_api_client_with_overrides, ApiClient}; use socket_patch_core::crawlers::{CrawlerOptions, Ecosystem}; use socket_patch_core::manifest::cleanup_blobs::ArtifactReferences; use socket_patch_core::manifest::operations::{ - get_before_hash_blobs, read_manifest, write_manifest, + get_before_hash_blobs, manifest_exists, read_manifest, write_manifest, }; use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord}; use socket_patch_core::patch::apply::select_installed_variants; @@ -1397,7 +1397,22 @@ pub async fn run(args: RollbackArgs) -> i32 { // not this run's to unwind (see `project_state_in_scope`): a global // rollback restores the global copies and their manifest records only. let project_state = crate::commands::project_state_in_scope(&args.common); - let manifest_missing = tokio::fs::metadata(&manifest_path).await.is_err(); + // Only a missing file is "no manifest"; one rollback cannot even stat + // fails like the load below, not as "Manifest not found" (#998). + let manifest_missing = match manifest_exists(&manifest_path).await { + Ok(present) => !present, + Err(e) => { + track_patch_rollback_failed(&e.to_string(), &telemetry).await; + let err = crate::json_envelope::manifest_load_error(&manifest_path, &e); + emit_rollback_error( + args.common.json, + args.common.dry_run, + &err.code, + &err.message, + ); + return 1; + } + }; let vendor_ledger_exists = project_state && tokio::fs::metadata(ledger_root.join(".socket/vendor/state.json")) .await diff --git a/crates/socket-patch-cli/src/commands/scan/gc.rs b/crates/socket-patch-cli/src/commands/scan/gc.rs index ab5eabcd7..b8252d001 100644 --- a/crates/socket-patch-cli/src/commands/scan/gc.rs +++ b/crates/socket-patch-cli/src/commands/scan/gc.rs @@ -3,7 +3,7 @@ //! read-only (preview) forms. use socket_patch_core::manifest::cleanup_blobs::{ArtifactReferences, CleanupResult}; -use socket_patch_core::manifest::operations::{read_manifest, write_manifest}; +use socket_patch_core::manifest::operations::{manifest_exists, read_manifest, write_manifest}; use socket_patch_core::manifest::schema::PatchManifest; use socket_patch_core::utils::purl_key::PurlKey; use socket_patch_core::vendor::{purl_keys_cover, VENDOR_STATE_REL}; @@ -74,6 +74,20 @@ impl GcSummary { gc } + /// The vendored half alone, for a pass whose manifest exists but could + /// not be loaded: the manifest half did not run, and the load error + /// (`manifest_invalid` / `manifest_unreadable`) joins `warnings` + /// instead of the pass reading as "no manifest, nothing to prune". + fn manifest_unloadable(v: VendorGcSummary, manifest_path: &Path, e: &std::io::Error) -> Self { + let mut gc = GcSummary::vendor_only(v); + let err = crate::json_envelope::manifest_load_error(manifest_path, e); + gc.warnings.push(( + crate::json_envelope::manifest_load_error_code(e), + err.message, + )); + gc + } + /// Fold a vendored-state GC pass into this summary: purl lists sorted, /// its failed post-revert rewrites join `warnings`. (`skipped` is this /// pass's alone — the vendored half runs under this pass's lock and @@ -231,9 +245,9 @@ pub(super) async fn run_apply_gc( // pristine checkout with neither a manifest nor a ledger must not gain // one. Either store alone is enough to proceed; the authoritative reads // happen under the lock. - let has_manifest = tokio::fs::metadata(manifest_path) - .await - .is_ok_and(|m| m.is_file()); + // A manifest that cannot be stated is not absent (#998): the locked + // read below reports it. + let has_manifest = manifest_exists(manifest_path).await.unwrap_or(true); let has_ledger = tokio::fs::metadata(common.cwd.join(VENDOR_STATE_REL)) .await .is_ok_and(|m| m.is_file()); @@ -261,11 +275,13 @@ pub(super) async fn run_apply_gc( // Vendored-state GC FIRST (see the fn doc), under this guard. let vendor_gc = run_vendor_gc(common, manifest_path, /*dry_run=*/ false).await; - // Authoritative read under the lock. Missing or unreadable ⇒ nothing - // to prune, and the blob sweep has no referenced-set to work from. + // Authoritative read under the lock. Missing ⇒ nothing to prune, and + // the blob sweep has no referenced-set to work from; unreadable ⇒ the + // same, but said (#1063). let mut manifest = match read_manifest(manifest_path).await { Ok(Some(m)) => m, - _ => return GcSummary::vendor_only(vendor_gc), + Ok(None) => return GcSummary::vendor_only(vendor_gc), + Err(e) => return GcSummary::manifest_unloadable(vendor_gc, manifest_path, &e), }; let mut prunable = detect_prunable(&manifest, scanned_purls, vendored); let kept = keep_patched_cargo_copies(common, &manifest, socket_dir, &mut prunable).await; @@ -384,7 +400,8 @@ async fn preview_apply_gc( let mut manifest = match read_manifest(manifest_path).await { Ok(Some(m)) => m, - _ => return GcSummary::vendor_only(vendor_gc), + Ok(None) => return GcSummary::vendor_only(vendor_gc), + Err(e) => return GcSummary::manifest_unloadable(vendor_gc, manifest_path, &e), }; // Mirror the wet pass, which drops an unused vendored entry's manifest // keys before the blob sweep, or the preview under-reports orphans. @@ -939,6 +956,65 @@ mod tests { (manifest_path, socket_dir, blob_path) } + /// #1063: a manifest that exists but cannot be loaded is not "no + /// manifest". Both passes skip the manifest half (nothing is pruned or + /// swept without a referenced set) and say why with the shared load + /// code, instead of a clean summary that reads as nothing to prune. + #[tokio::test] + async fn apply_gc_reports_an_unloadable_manifest() { + for (seed, code) in [ + ( + (|p: &std::path::Path| std::fs::write(p, r#"{"patches": {"#).unwrap()) + as fn(&std::path::Path), + "manifest_invalid", + ), + ( + |p: &std::path::Path| std::fs::create_dir_all(p).unwrap(), + "manifest_unreadable", + ), + ] { + for dry_run in [true, false] { + let tmp = tempfile::tempdir().unwrap(); + let socket_dir = tmp.path().join(".socket"); + let blob = socket_dir.join("blobs").join("a".repeat(64)); + std::fs::create_dir_all(blob.parent().unwrap()).unwrap(); + std::fs::write(&blob, b"orphan?").unwrap(); + let manifest_path = socket_dir.join("manifest.json"); + seed(&manifest_path); + + let common = gc_common(tmp.path()); + let gc = if dry_run { + preview_apply_gc( + &common, + &manifest_path, + &socket_dir, + &HashSet::new(), + &no_vendored(), + ) + .await + } else { + run_apply_gc( + &common, + &manifest_path, + &socket_dir, + &HashSet::new(), + &no_vendored(), + ) + .await + }; + let codes: Vec<&str> = gc.warnings.iter().map(|(c, _)| *c).collect(); + assert_eq!(codes, vec![code], "dry_run={dry_run}: {gc:?}"); + assert!(gc.skipped.is_none(), "dry_run={dry_run}: {gc:?}"); + assert!(gc.pruned.is_empty(), "dry_run={dry_run}: {gc:?}"); + assert_eq!(gc.blobs.blobs_removed, 0, "dry_run={dry_run}: {gc:?}"); + assert!( + blob.exists(), + "dry_run={dry_run}: no sweep without a manifest" + ); + } + } + } + #[tokio::test] async fn preview_apply_gc_reports_blobs_of_prunable_entry() { // The package is not installed (empty scan), so its entry is prunable diff --git a/crates/socket-patch-cli/src/commands/vendor.rs b/crates/socket-patch-cli/src/commands/vendor.rs index bd85b661f..7effb167a 100644 --- a/crates/socket-patch-cli/src/commands/vendor.rs +++ b/crates/socket-patch-cli/src/commands/vendor.rs @@ -24,7 +24,7 @@ use futures_util::StreamExt; use socket_patch_core::api::client::get_api_client_with_overrides; use socket_patch_core::constants::SOCKET_DIR; use socket_patch_core::crawlers::{CrawlerOptions, Ecosystem}; -use socket_patch_core::manifest::operations::{read_manifest, write_manifest}; +use socket_patch_core::manifest::operations::{manifest_exists, read_manifest, write_manifest}; use socket_patch_core::manifest::schema::{PatchManifest, PatchRecord}; use socket_patch_core::patch::apply::{verify_file_patch, PatchSources}; use socket_patch_core::patch::redirect::upstream::HostedPin; @@ -954,6 +954,21 @@ pub async fn run(args: VendorArgs) -> i32 { let manifest_path = args.common.resolved_manifest_path(); let socket_dir = crate::args::socket_dir_of(&manifest_path, &args.common.cwd); + // Only a missing file is "no manifest": one vendor cannot even stat + // is refused, never reported as a calm `noManifest` exit 0 (#998). + // Revert needs only the vendor ledger, so it never asks. + let manifest_present = if args.revert { + false + } else { + match manifest_exists(&manifest_path).await { + Ok(present) => present, + Err(e) => { + let err = crate::json_envelope::manifest_load_error(&manifest_path, &e); + return emit_eject_refusal(&args.common, &err.code, &err.message); + } + } + }; + // Hosted pins are the input, even when an agent manifest also exists. // The raw lockfile inventory does not require VEX eligibility or an // installed copy of a patch. Revert needs only the vendor ledger. @@ -966,8 +981,7 @@ pub async fn run(args: VendorArgs) -> i32 { let refusal = inventory.contested_refusal(); let pins = hosted_pins_in_scope(&args.common, inventory.pins); if let Some(refusal) = refusal { - let has_manifest = tokio::fs::metadata(&manifest_path).await.is_ok(); - if !has_manifest || !pins.is_empty() { + if !manifest_present || !pins.is_empty() { return emit_eject_refusal(&args.common, "hosted_wiring_contested", &refusal); } } @@ -993,7 +1007,7 @@ pub async fn run(args: VendorArgs) -> i32 { // Preserve legacy manifest-only vendoring when there are no hosted // pins. A missing manifest is a no-op; detached vendored projects keep // their records in the ledger instead. - if !args.revert && tokio::fs::metadata(&manifest_path).await.is_err() { + if !args.revert && !manifest_present { // A requested `--vex` still attests what the `.socket/vendor` // ledgers and lockfiles already wire. Same contract as `apply --vex` // with no manifest: nothing referenced anywhere keeps exit 0; any diff --git a/crates/socket-patch-cli/tests/apply/apply_invariants.rs b/crates/socket-patch-cli/tests/apply/apply_invariants.rs index b2d8ca66b..ac613dc4f 100644 --- a/crates/socket-patch-cli/tests/apply/apply_invariants.rs +++ b/crates/socket-patch-cli/tests/apply/apply_invariants.rs @@ -491,10 +491,6 @@ fn apply_with_no_socket_dir_silent_emits_nothing() { /// which the install hook and CI both read as success. Fail closed. #[cfg(unix)] #[test] -#[ignore = "RED: apply's manifest probe is `tokio::fs::metadata(..).is_err()`, so \ - an UNREADABLE manifest is reported as the clean `noManifest` no-op \ - (exit 0) exactly like a missing one — the install hook and CI both \ - read that as success. The fail-closed fix was not part of this change."] fn apply_with_unreadable_socket_dir_fails_closed() { use std::os::unix::fs::PermissionsExt; diff --git a/crates/socket-patch-cli/tests/cli_parse_list.rs b/crates/socket-patch-cli/tests/cli_parse_list.rs index 741db07d7..e5c5d7682 100644 --- a/crates/socket-patch-cli/tests/cli_parse_list.rs +++ b/crates/socket-patch-cli/tests/cli_parse_list.rs @@ -380,14 +380,12 @@ fn missing_manifest_under_valid_cwd_is_not_an_error_via_binary() { } #[test] -fn manifest_path_is_existing_directory_is_a_usage_error_via_binary() { - // A `--manifest-path` naming a directory is never a manifest: v5.0 - // rejects it before the command runs, as a usage error (exit 2, the - // `Error:` line naming the flag), on every project command - // (`GlobalArgs::validate_paths`). Before, `list` reached `read_manifest` - // and failed with `manifest_unreadable` (exit 1); the I/O - // classification itself (a non-absence error is never - // `manifest_not_found`) is pinned on `read_manifest` in core. +fn manifest_path_is_existing_directory_is_manifest_unreadable_via_binary() { + // A `--manifest-path` naming a directory is a corrupt project, not a + // bad flag: `read_manifest` refuses it as `manifest_unreadable` (exit 1, + // with the `--json` envelope), the same verdict the default path gets. + // Refusing it in `GlobalArgs::validate_paths` made the verdict depend + // on whether the path was spelled out (#1123). let tmp = tempfile::tempdir().unwrap(); let manifest_path = tmp.path().join("manifest-is-a-dir"); std::fs::create_dir(&manifest_path).unwrap(); @@ -396,12 +394,12 @@ fn manifest_path_is_existing_directory_is_a_usage_error_via_binary() { tmp.path(), &["--json", "--manifest-path", manifest_path.to_str().unwrap()], ); + let stdout = String::from_utf8_lossy(&out.stdout); let stderr = String::from_utf8_lossy(&out.stderr); - assert_eq!(out.status.code(), Some(2), "stderr={stderr}"); - assert!( - stderr.contains("--manifest-path") && stderr.contains("is a directory"), - "{stderr}" - ); + assert_eq!(out.status.code(), Some(1), "stderr={stderr}"); + let v: serde_json::Value = serde_json::from_str(stdout.trim()) + .unwrap_or_else(|e| panic!("stdout must be one envelope ({e}): {stdout}")); + assert_eq!(v["error"]["code"], "manifest_unreadable", "envelope: {v}"); } // --------------------------------------------------------------------------- diff --git a/crates/socket-patch-cli/tests/cli_path_flags_validated.rs b/crates/socket-patch-cli/tests/cli_path_flags_validated.rs index 2e01d22e7..df2c29420 100644 --- a/crates/socket-patch-cli/tests/cli_path_flags_validated.rs +++ b/crates/socket-patch-cli/tests/cli_path_flags_validated.rs @@ -121,8 +121,8 @@ fn nonexistent_global_prefix_is_a_usage_error_on_every_command() { } /// A `--manifest-path` in a directory that does not exist fails every -/// command with exit 2 (`list` used to say "No patches" and exit 0); one -/// naming a directory fails too. +/// command with exit 2 (`list` used to say "No patches" and exit 0). One +/// naming a directory is a manifest that cannot be read, not a bad flag. #[test] fn manifest_path_in_a_missing_directory_is_a_usage_error_on_every_command() { let tmp = tempfile::tempdir().unwrap(); @@ -142,13 +142,16 @@ fn manifest_path_in_a_missing_directory_is_a_usage_error_on_every_command() { ); assert!(stderr.contains("--manifest-path"), "{command:?}: {stderr}"); } + // A directory is an existing (unreadable) manifest, not a bad flag: + // `list` runs and refuses it (exit 1), as it does under the default + // path (#1123). let (code, _stdout, stderr) = run( tmp.path(), &["list"], &["--manifest-path", tmp.path().to_str().unwrap()], ); - assert_eq!(code, 2, "a directory is no manifest file: {stderr}"); - assert!(stderr.contains("is a directory"), "{stderr}"); + assert_eq!(code, 1, "a directory is an unreadable manifest: {stderr}"); + assert!(!stderr.contains("--manifest-path"), "{stderr}"); } /// Anti-vacuous controls: real directories still run, and a missing @@ -191,7 +194,9 @@ fn existing_paths_and_a_missing_manifest_file_still_run() { /// The commands that create the manifest keep creating its directory: a /// fresh checkout's `get --manifest-path state/patches.json` is no usage -/// error, and neither is `scan`. A directory in that position still is. +/// error, and neither is `scan`. A directory in that position is no usage +/// error either, but the run still fails: it is an unreadable manifest +/// (#1123). #[test] fn manifest_creating_commands_accept_a_manifest_path_in_a_new_directory() { let tmp = tempfile::tempdir().unwrap(); @@ -212,10 +217,11 @@ fn manifest_creating_commands_accept_a_manifest_path_in_a_new_directory() { command, &["--manifest-path", tmp.path().to_str().unwrap()], ); - assert_eq!( - code, 2, + assert_ne!( + code, 0, "{command:?}: a directory is no manifest file: {stderr}" ); + assert!(!stderr.contains("--manifest-path"), "{command:?}: {stderr}"); } } From 91d884a12091442e805b8d1db72cd92ae7dd0637 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 09:18:47 +0000 Subject: [PATCH 4/5] Document manifest_unreadable for stat errors The contract's manifest_unreadable row now says a path that cannot be stated is unreadable, never noManifest or manifest_not_found, and that the verdict does not depend on how the path is spelled. The ENOTDIR matrix case accepts vendor --check refusing the vendor ledger under the same unreadable .socket, which it reads first. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- crates/socket-patch-cli/src/commands/scan/gc.rs | 4 +++- .../tests/manifest_load_error_matrix.rs | 15 ++++++++++----- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 5acc22d01..3e385deab 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -1734,7 +1734,7 @@ outcome for a selected record, rather than inferring a write from exit 0. | `manifest_not_found` | remove, repair, rollback, vex, apply (only when the manifest vanishes between apply's existence probe and its locked read) (not `list` since v5.0: a missing manifest is an empty list) | `.socket/manifest.json` doesn't exist. For `vex` (and `scan --vex`) it fires only when, in addition, NOTHING else names a patch — no vendor-ledger entry, no lockfile reference (hosted or vendored) — and the message says so (exit 2 standalone; `apply`/`vendor --vex` treat it as their calm no-op). v3.5: `repair` proceeds anyway (vendored phase only) when a vendor ledger or vendor-path lockfile references exist, and exits 0 with a `redirect_only_project` skip (not this error) when the project's only patch state is hosted pins in its lockfiles (v5.0; or a pre-v5 `redirect-state.json`). `list` likewise no longer fires this on a hosted-only project: v5.0 lists every hosted pin the lockfiles wire (exit 0, labeled `details.mode: "hosted"` + `details.lockfiles: []` — no `details.ledger`, since hosted mode keeps none; when the manifest exists too, both are shown, purl-sorted with the manifest entry first on a tie). A pin carries its uuid and empty details unless a pre-v5 redirect ledger records the same purl and uuid (read for migration only: its record supplies the vulnerabilities / tier / description); a pre-v5 ledger record whose pin is in no lockfile is not listed. v5.0: `list` reads the vendor ledger the same way — a vendored-only project (every `scan`/`get --mode vendored` project) lists its ledger entries' embedded records labeled `Mode: vendored (recorded in .socket/vendor/state.json)` in human mode — the twin of the hosted `Mode: hosted (wired in )` line — (`details.mode: "vendored"` + `details.ledger: ".socket/vendor/state.json"` in JSON), exit 0. A standalone-`vendor` entry's fallback `record` lists the same way once no manifest entry covers it (by ledger key or base purl) — the copy manifest-less `vex` attests from, so `list` never reports `manifest_not_found` for a tree whose VEX document attests a patch; while the manifest covers it, only the manifest entry is listed. All sources always come from the SAME project (see the `--manifest-path` flag row), so `--manifest-path` into another project reads that project's state, never the local one. The error still fires when NONE of the three sources has a patch, and a present-but-broken manifest still reports `manifest_invalid`/`manifest_unreadable` regardless (corruption is never masked). A malformed pre-v5 redirect ledger degrades to "nothing to consult" with a stderr warning, muted by `--silent` (the pins still list); `list --json` carries it in the run-level `warnings[]` as `redirect_ledger_corrupt` instead of on stderr. v5.0: `rollback` likewise proceeds manifest-less when the vendor ledger or the lockfiles' hosted pins hold work (its error is the legacy `{status: "error", error: {code: "manifest_not_found", message: "Manifest not found"}, path}` shape); only the truly-empty project — no manifest, no vendor ledger, no hosted pin (a lone pre-v5 redirect ledger is deleted, exit 0) — keeps the exit-1 error, and a project whose lockfiles still reference `.socket/vendor/` artifacts with NO vendor ledger gets a distinct error naming `socket-patch repair`. `remove` (v5.0) proceeds manifest-less whenever a vendor ledger file exists or the lockfiles pin a hosted patch (an existence probe and the read-only hosted-pin discovery before the lock; the vendor ledger loads under it): ANY vendor-ledger entry matching the identifier — detached or not — is removed through the ledger path (`--preserve-state` and drift-keeps behave exactly as on the manifest path), a hosted-only match restores its upstream registry entry, and when that state exists but holds nothing for the identifier the error is `not_found` (exit 1), not this code — `manifest_not_found` fires from `remove` only when all three sources are empty. Manifest entries are removed in sorted purl order. | | `ambiguous_target` | remove, get, rollback | The identifier is a package name whose last-segment rule selects several packages across the manifest, the vendor ledger and the hosted pins (`core` → `@angular/core` and `@babel/core`). Nothing is changed (exit 1); the message names each package. Use the full name or a purl. See **Target grammar**. | | `manifest_invalid` | list, remove, apply (incl. `--check`), repair, vendor (incl. `--check`), vex, rollback, get, scan (agent mode) | Manifest exists but is unparseable (malformed JSON or a schema violation). **v5.0, MAJOR (#931)**: one mapping on every command — `apply` used to say `apply_failed`, `repair` `repair_failed`, `vendor` the undocumented `invalid_manifest`, and `apply --check` / `vendor --check` / `vex` / `get` / `rollback` `manifest_unreadable`. Exit codes unchanged (`vex`: 2, every other command 1). Agent-mode `scan` fails with it (exit 1); the other scan modes, which don't read the manifest, add a warning with the same code and go on. | -| `manifest_unreadable` | list, remove, apply (incl. `--check`), repair, vendor (incl. `--check`), vex, rollback, get, scan (agent mode) | Manifest exists but cannot be read (an I/O error: permission denied, a directory at the path, …). `vex`: exit 2. Other scan modes warn, as for `manifest_invalid`. | +| `manifest_unreadable` | list, remove, apply (incl. `--check`), repair, vendor (incl. `--check`), vex, rollback, get, scan (agent mode) | Manifest exists but cannot be read (an I/O error: permission denied, a directory at the path, …). Only a missing file is absence: a path that cannot even be stated (an unsearchable `.socket/`, `.socket` as a plain file, a symlink loop) is this code too, never `noManifest` or `manifest_not_found` (#998), and the verdict is the same whether the path is the default, spelled with `--manifest-path` or set through `SOCKET_MANIFEST_PATH` (#1123). `vex`: exit 2. Other scan modes warn, as for `manifest_invalid`, and so does a `scan --prune` GC pass that finds the manifest unloadable under its lock (#1063). | | `manifest_write_failed` | get | The manifest write after a download failed; the blobs that run wrote are removed again. | | `patch_fetch_failed` | get | The patch search or view request failed. | | `patch_no_applicable_files` | get | The fetched patch has no file it could record; nothing written. | diff --git a/crates/socket-patch-cli/src/commands/scan/gc.rs b/crates/socket-patch-cli/src/commands/scan/gc.rs index b8252d001..120a9e2ec 100644 --- a/crates/socket-patch-cli/src/commands/scan/gc.rs +++ b/crates/socket-patch-cli/src/commands/scan/gc.rs @@ -56,7 +56,9 @@ pub(super) struct GcSummary { /// mutations already happened on disk, so the stale record is reported, /// not the pass failed. Also the vendored reverts' reinstall advisories /// (`vendor_bun_reinstall_required`, `vendor_vlt_reinstall_required`) - /// and no other revert warning. Serialized as additive + /// and no other revert warning. And `manifest_invalid` / + /// `manifest_unreadable` when the manifest half could not load the + /// manifest (#1063). Serialized as additive /// `warnings[]` on the apply shape only. warnings: Vec<(&'static str, String)>, } diff --git a/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs b/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs index 44a47bbbc..b29447a50 100644 --- a/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs +++ b/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs @@ -93,13 +93,13 @@ enum Spelling { } fn assert_matrix(seed: impl Fn(&Path), code: &str) { - assert_matrix_in(project, &seed, code, Spelling::Default); + assert_matrix_in(project, &seed, &[code], Spelling::Default); } fn assert_matrix_in( make: fn(&dyn Fn(&Path)) -> tempfile::TempDir, seed: &dyn Fn(&Path), - code: &str, + codes: &[&str], spelling: Spelling, ) { for (argv, command, exit) in MATRIX { @@ -121,7 +121,10 @@ fn assert_matrix_in( let v: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap_or_else(|e| { panic!("{argv:?}: stdout must be one JSON envelope ({e}): {stdout:?}\nstderr={stderr}") }); - assert_eq!(v["error"]["code"], code, "{argv:?}: {v}"); + assert!( + codes.iter().any(|c| v["error"]["code"] == *c), + "{argv:?}: expected one of {codes:?}: {v}" + ); assert_eq!(got_exit, *exit, "{argv:?}: {v}\nstderr={stderr}"); // A full envelope, not a bare `{status, error}`. assert_eq!(v["command"], *command, "{argv:?}: {v}"); @@ -166,7 +169,9 @@ fn socket_dir_as_file_is_manifest_unreadable_on_every_command() { assert_matrix_in( project_root, &|root| std::fs::write(root.join(".socket"), "not a directory").unwrap(), - "manifest_unreadable", + // `vendor --check` reads the vendor ledger under the same `.socket` + // first, and refuses that store just as closed. + &["manifest_unreadable", "vendor_state_unreadable"], Spelling::Default, ); } @@ -194,7 +199,7 @@ fn directory_manifest_verdict_does_not_depend_on_spelling() { assert_matrix_in( project, &|path| std::fs::create_dir_all(path).unwrap(), - "manifest_unreadable", + &["manifest_unreadable"], spelling, ); } From 1f3e0babad1af973428af8cf182110c44357e24d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 09:35:44 +0000 Subject: [PATCH 5/5] Address review: Windows, contract, check message On Windows a path under a plain file reports as not found, so the .socket-as-a-file cases are Unix-only tests (read_manifest and manifest_exists agree there either way). manifest_exists now wraps tokio::fs::try_exists. The exit-2 contract row no longer lists a --manifest-path that names a directory. apply --check keeps its "treating it as drift" message whether the manifest fails to stat or to load, repair records failure telemetry for the early refusal like its load failure, and the GC warning skips a throwaway envelope. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- crates/socket-patch-cli/src/commands/apply.rs | 32 +++++++------------ .../socket-patch-cli/src/commands/repair.rs | 4 +++ .../socket-patch-cli/src/commands/scan/gc.rs | 8 ++--- .../tests/manifest_load_error_matrix.rs | 4 ++- .../src/manifest/operations.rs | 17 ++++++---- 6 files changed, 35 insertions(+), 32 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 3e385deab..ac0324985 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -2071,7 +2071,7 @@ Exit `1` when `status` is `partialFailure` (any `events[*].action == "failed"`) |---|---| | `0` | Success | | `1` | Error (missing/invalid manifest, fetch failed, apply failed, selection cancelled in non-JSON mode, an invalid or ambiguous socket.yml on `scan` (v5.0), etc.) | -| `2` | Usage error: clap parse failures (unknown flag/value, missing required arg, an unknown subcommand such as the removed `setup`) and the conflicts the commands enforce themselves — `scan`'s cross-mode conflict (`--sync` combined with a `--mode` other than `agent`, rejected in `resolve_mode_flags`) and `--mode hosted\|vendored` with `--global`/`--global-prefix` (same enforcement point; `get` refuses the same combination) or with a `--manifest-path` in another project (v5.0, `manifest_path_foreign_project`; `get` and `vendor` other than `--check` likewise, as do `apply --vex` and any other `scan --vex`); a `scan`/`get` with no `--mode` on a project holding both agent and vendored state (`mode_ambiguous`, v5.0); in hosted/vendored `scan` (bare `scan` included), a PATH that is not a directory, a PATH glob matching no directory, and `--json` with more than one project directory (`run_project_dirs`); on every project command (all but `--update`), a `--cwd` / `SOCKET_CWD` or `--global-prefix` / `SOCKET_GLOBAL_PREFIX` that is not an existing directory, and a non-default `--manifest-path` / `SOCKET_MANIFEST_PATH` that names a directory or sits in a project directory that does not exist (v5.0; checked once before the command runs, whether or not the command uses the flag — previously these read as an empty project and exited 0, and `remove` / `rollback` exited 1; a missing manifest FILE in an existing project stays legal, and `get` / `scan`, which create the manifest, still create a missing manifest directory); `remove --preserve-state --skip-rollback` (the no-op quadrant; flag- or env-sourced alike), an unparseable path glob on `scan`/`rollback`, a `scan` PATH outside the repository root and a malformed `SOCKET_MIN_SEVERITY` or `SOCKET_MAX_NEW_PATCHES` (v5.0), `repair --offline --download-only`. Under `--json`, every usage error a command enforces itself prints the coded error on stdout (v5.0, MAJOR: `scan`, `remove` and `rollback` printed nothing there): a full envelope for the envelope commands, `{status: "error", error: {code, message}}` for `scan`, `get` and `rollback`; clap's own parse errors, and the path-flag check that runs before every command (`--cwd` / `--global-prefix` / `--manifest-path` naming nothing), still print nothing on stdout. `vex` also exits `2` on hard errors before document generation (see its tri-state table below). v5.0: `get`'s self-enforced conflicts exit `2` too (`--id`/`--cve`/`--ghsa`/`--package` multi-select, `--mode hosted\|vendored --save-only`, a malformed identifier for a forced `--id`/`--cve`/`--ghsa`) — previously `1` (MAJOR). The never-implemented `get --one-off` / `rollback --one-off` (and `SOCKET_ONE_OFF`) are removed in v5.0; `--one-off` is now an ordinary unknown-flag clap error. | +| `2` | Usage error: clap parse failures (unknown flag/value, missing required arg, an unknown subcommand such as the removed `setup`) and the conflicts the commands enforce themselves — `scan`'s cross-mode conflict (`--sync` combined with a `--mode` other than `agent`, rejected in `resolve_mode_flags`) and `--mode hosted\|vendored` with `--global`/`--global-prefix` (same enforcement point; `get` refuses the same combination) or with a `--manifest-path` in another project (v5.0, `manifest_path_foreign_project`; `get` and `vendor` other than `--check` likewise, as do `apply --vex` and any other `scan --vex`); a `scan`/`get` with no `--mode` on a project holding both agent and vendored state (`mode_ambiguous`, v5.0); in hosted/vendored `scan` (bare `scan` included), a PATH that is not a directory, a PATH glob matching no directory, and `--json` with more than one project directory (`run_project_dirs`); on every project command (all but `--update`), a `--cwd` / `SOCKET_CWD` or `--global-prefix` / `SOCKET_GLOBAL_PREFIX` that is not an existing directory, and a non-default `--manifest-path` / `SOCKET_MANIFEST_PATH` that sits in a project directory that does not exist (v5.0; checked once before the command runs, whether or not the command uses the flag — previously these read as an empty project and exited 0, and `remove` / `rollback` exited 1; a missing manifest FILE in an existing project stays legal, and `get` / `scan`, which create the manifest, still create a missing manifest directory; one that names a directory is an unreadable manifest, `manifest_unreadable` exit 1 like the default path, not a usage error, #1123); `remove --preserve-state --skip-rollback` (the no-op quadrant; flag- or env-sourced alike), an unparseable path glob on `scan`/`rollback`, a `scan` PATH outside the repository root and a malformed `SOCKET_MIN_SEVERITY` or `SOCKET_MAX_NEW_PATCHES` (v5.0), `repair --offline --download-only`. Under `--json`, every usage error a command enforces itself prints the coded error on stdout (v5.0, MAJOR: `scan`, `remove` and `rollback` printed nothing there): a full envelope for the envelope commands, `{status: "error", error: {code, message}}` for `scan`, `get` and `rollback`; clap's own parse errors, and the path-flag check that runs before every command (`--cwd` / `--global-prefix` / `--manifest-path` naming nothing), still print nothing on stdout. `vex` also exits `2` on hard errors before document generation (see its tri-state table below). v5.0: `get`'s self-enforced conflicts exit `2` too (`--id`/`--cve`/`--ghsa`/`--package` multi-select, `--mode hosted\|vendored --save-only`, a malformed identifier for a forced `--id`/`--cve`/`--ghsa`) — previously `1` (MAJOR). The never-implemented `get --one-off` / `rollback --one-off` (and `SOCKET_ONE_OFF`) are removed in v5.0; `--one-off` is now an ordinary unknown-flag clap error. | `list` returns **`0`** for every project it can read, empty or not (**v5.0, BREAKING**: a project with no manifest and no ledger record — normal for hosted mode, which writes no manifest — used to exit `1` with `manifest_not_found`; it is now an empty list: `No patches in this project. Run \`socket-patch scan\`.` on stdout, and under `--json` the success envelope with `events: []`). Only an unreadable or invalid manifest (`manifest_unreadable` / `manifest_invalid`) exits `1`. Every lock-taking subcommand — including `scan`/`get --mode hosted` as of v5.0 — returns **`1`** with `error.code: lock_held` when another live socket-patch process holds `<.socket>/apply.lock`. diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index c4c098be3..5c16e2423 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -318,25 +318,7 @@ async fn run_check(args: &ApplyArgs, manifest_path: &Path) -> i32 { } return 0; } - Err(e) => { - let msg = format!( - "Patch check could not read the manifest ({e}); \ - treating it as drift (fail-closed)." - ); - if args.common.json { - let mut env = Envelope::new(Command::Apply); - env.dry_run = args.common.dry_run; - let mut err = crate::json_envelope::manifest_load_error(manifest_path, &e); - err.message = msg; - env.mark_error(err); - println!("{}", env.to_pretty_json()); - } else { - // Errors print even under --silent ("errors only", never - // "nothing"): exit 1 with no message would be undiagnosable. - eprintln!("Error: {msg}"); - } - return 1; - } + Err(e) => return refuse_unreadable_manifest(args, manifest_path, &e), }; // (purl_or_name, reason_code, detail) for each drift. @@ -871,14 +853,24 @@ const YARN_PNP_UNSUPPORTED: &str = "yarn-berry Plug'n'Play layout is not support /// A manifest that exists but cannot be stated, refused before the API /// client, the lock or telemetry are touched: the same envelope and exit /// as the load failure further down (#931, #998). +/// `--check` says so as the drift it is treated as, whether the manifest +/// failed to stat here or to load in [`run_check`]. fn refuse_unreadable_manifest(args: &ApplyArgs, manifest_path: &Path, e: &std::io::Error) -> i32 { - let err = crate::json_envelope::manifest_load_error(manifest_path, e); + let mut err = crate::json_envelope::manifest_load_error(manifest_path, e); + if args.check { + err.message = format!( + "Patch check could not read the manifest ({e}); \ + treating it as drift (fail-closed)." + ); + } if args.common.json { let mut env = Envelope::new(Command::Apply); env.dry_run = args.common.dry_run; env.mark_error(err); println!("{}", env.to_pretty_json()); } else { + // Errors print even under --silent ("errors only", never + // "nothing"): exit 1 with no message would be undiagnosable. eprintln!("Error: {}", crate::ui::sentence_case(&err.message)); } 1 diff --git a/crates/socket-patch-cli/src/commands/repair.rs b/crates/socket-patch-cli/src/commands/repair.rs index bd9436638..929987f43 100644 --- a/crates/socket-patch-cli/src/commands/repair.rs +++ b/crates/socket-patch-cli/src/commands/repair.rs @@ -71,6 +71,10 @@ pub async fn run(args: RepairArgs) -> i32 { Ok(present) => present, Err(e) => { let err = crate::json_envelope::manifest_load_error(&manifest_path, &e); + // Recorded like the load failure in `repair_inner` (no client + // is built before this gate). + track_patch_repair_failed(&err.message, &TelemetryAuth::from_credentials(None, None)) + .await; if args.common.json { let env = error_envelope( Command::Repair, diff --git a/crates/socket-patch-cli/src/commands/scan/gc.rs b/crates/socket-patch-cli/src/commands/scan/gc.rs index 120a9e2ec..a013aac58 100644 --- a/crates/socket-patch-cli/src/commands/scan/gc.rs +++ b/crates/socket-patch-cli/src/commands/scan/gc.rs @@ -82,10 +82,9 @@ impl GcSummary { /// instead of the pass reading as "no manifest, nothing to prune". fn manifest_unloadable(v: VendorGcSummary, manifest_path: &Path, e: &std::io::Error) -> Self { let mut gc = GcSummary::vendor_only(v); - let err = crate::json_envelope::manifest_load_error(manifest_path, e); gc.warnings.push(( crate::json_envelope::manifest_load_error_code(e), - err.message, + crate::ui::manifest_error_message(manifest_path, e), )); gc } @@ -247,8 +246,9 @@ pub(super) async fn run_apply_gc( // pristine checkout with neither a manifest nor a ledger must not gain // one. Either store alone is enough to proceed; the authoritative reads // happen under the lock. - // A manifest that cannot be stated is not absent (#998): the locked - // read below reports it. + // A manifest that cannot be stated is not absent (#998): the pass goes + // on and reports it, as the locked read's load error or, when + // `.socket/` itself is unusable, as the lock failure. let has_manifest = manifest_exists(manifest_path).await.unwrap_or(true); let has_ledger = tokio::fs::metadata(common.cwd.join(VENDOR_STATE_REL)) .await diff --git a/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs b/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs index b29447a50..fe912e751 100644 --- a/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs +++ b/crates/socket-patch-cli/tests/manifest_load_error_matrix.rs @@ -163,7 +163,9 @@ fn unreadable_manifest_is_manifest_unreadable_on_every_command() { /// #998: `.socket` is a plain file, so stating the manifest fails with /// ENOTDIR. That is unreadable, not missing: `apply` and `vendor` used to /// report `noManifest` and exit 0, the others `manifest_not_found`. Runs -/// as root, unlike a `chmod 000` fixture. +/// as root, unlike a `chmod 000` fixture. Unix only: Windows reports a +/// path under a file as not found, so there it is no manifest. +#[cfg(unix)] #[test] fn socket_dir_as_file_is_manifest_unreadable_on_every_command() { assert_matrix_in( diff --git a/crates/socket-patch-core/src/manifest/operations.rs b/crates/socket-patch-core/src/manifest/operations.rs index c98b6b50f..74a680e2f 100644 --- a/crates/socket-patch-core/src/manifest/operations.rs +++ b/crates/socket-patch-core/src/manifest/operations.rs @@ -74,12 +74,11 @@ pub async fn read_manifest( /// "no manifest" path never runs for a manifest it merely cannot read. /// Something that is not a regular file still counts as present: /// [`read_manifest`] then refuses it, whatever the path's spelling. +/// +/// Windows reports a path under a plain file as not found, so there (as in +/// [`read_manifest`]) `.socket` as a file reads as no manifest. pub async fn manifest_exists(path: impl AsRef) -> Result { - match tokio::fs::metadata(path.as_ref()).await { - Ok(_) => Ok(true), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), - Err(e) => Err(e), - } + tokio::fs::try_exists(path.as_ref()).await } /// Write a manifest to the filesystem with pretty-printed JSON. @@ -503,8 +502,14 @@ mod tests { std::fs::create_dir_all(&as_dir).unwrap(); assert!(manifest_exists(&as_dir).await.unwrap()); assert!(read_manifest(&as_dir).await.is_err()); + } - // `.socket` as a plain file: ENOTDIR, not NotFound. + // `.socket` as a plain file: ENOTDIR, not NotFound (Windows reports + // such a path as not found, for both functions alike). + #[cfg(unix)] + #[tokio::test] + async fn test_manifest_exists_under_a_file_is_error() { + let dir = tempfile::tempdir().unwrap(); let socket_file = dir.path().join("socket-file"); std::fs::write(&socket_file, "x").unwrap(); let under_file = socket_file.join("manifest.json");