Fix UTF-16 requirements.txt silently skipped (#721) - #724
Open
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Open
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Conversation
Assisted-by: Claude Code:claude-opus-5-5
Windows PowerShell 5.1 writes `pip freeze > requirements.txt` as UTF-16 with a byte-order mark, and pip installs from it. Hosted scan read every candidate file as UTF-8 and treated a file it could not decode as missing, so the run exited 0 as a success with nothing pinned, and pip kept installing the unpatched release. A fresh checkout's lock-only scan said "No packages found" for the same file. Hosted runs (disk and in-memory alike) now refuse with candidate_file_unreadable, naming the file and asking for it to be re-saved as UTF-8, whenever a non-UTF-8 candidate file belongs to an ecosystem being redirected. Nothing is written. Lock-only discovery decodes requirements.txt and its -r includes by BOM the way pip does, so the pins are found. Fixes #721 Assisted-by: Claude Code:claude-opus-5-5
Mikola Lysenko (mikolalysenko)
marked this pull request as ready for review
October 3, 2026 22:16
Collaborator
Author
|
BugBot review Generated by Claude Code |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
A vendored project switching to hosted mode had its vendored wiring reverted before the new non-UTF-8 check ran. A UTF-16 requirements.txt then refused the run with the vendored package already unwired, so it installed unpatched in both modes, and --dry-run predicted success. The check now runs before any revert, wet or dry. Vendored mode also named only "cannot read" for a UTF-16 root requirements.txt and silently skipped a UTF-16 -r include that pip installs from. Both now refuse by name with a re-save-as-UTF-8 hint. Refs #721 Assisted-by: Claude Code:claude-opus-5-5
Collaborator
Author
|
BugBot review Generated by Claude Code |
|
Bugbot Autofix prepared a fix for the issue found in the latest run.
Or push these changes by commenting: Preview (a9d1c657c4)diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs
--- a/crates/socket-patch-cli/src/commands/apply.rs
+++ b/crates/socket-patch-cli/src/commands/apply.rs
@@ -2,9 +2,7 @@
use socket_patch_core::api::blob_fetcher::get_missing_blobs;
use socket_patch_core::api::client::{get_api_client_with_overrides, ApiClient};
use socket_patch_core::crawlers::ruby_crawler::config_path_ignored_warning;
-use socket_patch_core::crawlers::{
- detect_npm_pkg_manager, Ecosystem, NpmPkgManager, RubyCrawler,
-};
+use socket_patch_core::crawlers::{detect_npm_pkg_manager, Ecosystem, NpmPkgManager, RubyCrawler};
use socket_patch_core::manifest::operations::read_manifest;
use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord};
use socket_patch_core::patch::apply::{
diff --git a/crates/socket-patch-cli/src/commands/list.rs b/crates/socket-patch-cli/src/commands/list.rs
--- a/crates/socket-patch-cli/src/commands/list.rs
+++ b/crates/socket-patch-cli/src/commands/list.rs
@@ -431,7 +431,10 @@
detail: detail.clone(),
});
} else if !args.common.silent {
- eprintln!("Warning: {}", crate::commands::rollback::capitalize_first(detail));
+ eprintln!(
+ "Warning: {}",
+ crate::commands::rollback::capitalize_first(detail)
+ );
}
}
let vendor_state = crate::commands::vendor_state_lenient(&loaded.vendor, args.common.silent);
@@ -773,12 +776,18 @@
let listings = HostedListing::from_pins(
&[
pin("pkg:npm/minimist@1.2.2", &record.uuid),
- pin("pkg:npm/other@1.0.0", "33333333-3333-4333-8333-333333333333"),
+ pin(
+ "pkg:npm/other@1.0.0",
+ "33333333-3333-4333-8333-333333333333",
+ ),
],
Some(&legacy),
);
assert_eq!(listings[0].record, record);
- assert_eq!(listings[1].record.uuid, "33333333-3333-4333-8333-333333333333");
+ assert_eq!(
+ listings[1].record.uuid,
+ "33333333-3333-4333-8333-333333333333"
+ );
assert!(listings[1].record.vulnerabilities.is_empty());
assert_eq!(listings[1].lockfiles, vec!["yarn.lock".to_string()]);
}
diff --git a/crates/socket-patch-cli/src/commands/mod.rs b/crates/socket-patch-cli/src/commands/mod.rs
--- a/crates/socket-patch-cli/src/commands/mod.rs
+++ b/crates/socket-patch-cli/src/commands/mod.rs
@@ -1,7 +1,7 @@
pub mod apply;
pub(crate) mod bun_preflight;
+pub(crate) mod composer_hints;
pub(crate) mod context;
-pub(crate) mod composer_hints;
pub(crate) mod fetch_stage;
pub mod get;
pub mod hosted_bundle;
@@ -9,11 +9,11 @@
pub(crate) mod lock_cli;
pub mod remove;
pub mod repair;
-pub(crate) mod vendored_backend;
pub mod rollback;
pub mod scan;
pub mod update;
pub mod vendor;
+pub(crate) mod vendored_backend;
pub mod vex;
pub(crate) mod vex_consumed;
pub(crate) mod vex_sources;
@@ -141,9 +141,11 @@
common: &crate::args::GlobalArgs,
root: &Path,
) -> socket_patch_core::patch::redirect::RedirectState {
- hosted_state_from_pins(&socket_patch_core::patch::redirect::upstream::HostedPin::all(
- &discover_wiring(common, root).await,
- ))
+ hosted_state_from_pins(
+ &socket_patch_core::patch::redirect::upstream::HostedPin::all(
+ &discover_wiring(common, root).await,
+ ),
+ )
}
/// [`hosted_state_from_lockfiles`] over already-discovered pins. A purl
@@ -153,10 +155,8 @@
) -> socket_patch_core::patch::redirect::RedirectState {
let mut state = socket_patch_core::patch::redirect::RedirectState::new();
for pin in pins {
- state
- .records
- .entry(pin.purl.clone())
- .or_insert_with(|| socket_patch_core::manifest::schema::PatchRecord {
+ state.records.entry(pin.purl.clone()).or_insert_with(|| {
+ socket_patch_core::manifest::schema::PatchRecord {
uuid: pin.uuid.clone(),
exported_at: String::new(),
files: Default::default(),
@@ -164,7 +164,8 @@
description: String::new(),
license: String::new(),
tier: String::new(),
- });
+ }
+ });
}
state
}
@@ -191,4 +192,3 @@
}
}
}
-
diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs
--- a/crates/socket-patch-cli/src/commands/remove.rs
+++ b/crates/socket-patch-cli/src/commands/remove.rs
@@ -17,9 +17,9 @@
pin_before_hash_blobs, rollback_patches_inner, run_hosted_leg, sweep_failure,
sweep_unused_artifacts, HostedLegOutcome, InnerSelection,
};
-use crate::commands::vendored_backend::{RevertedEntry, VendorRevertStep, VendoredBackend};
use crate::args::{apply_env_toggles, GlobalArgs};
use crate::commands::lock_cli::acquire_or_emit;
+use crate::commands::vendored_backend::{RevertedEntry, VendorRevertStep, VendoredBackend};
use crate::json_envelope::{Command, Envelope, EnvelopeError, PatchAction, PatchEvent, Status};
use crate::ui::plural;
diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs
--- a/crates/socket-patch-cli/src/commands/rollback.rs
+++ b/crates/socket-patch-cli/src/commands/rollback.rs
@@ -10,13 +10,13 @@
};
use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord};
use socket_patch_core::patch::apply::select_installed_variants;
+use socket_patch_core::patch::redirect::upstream::HostedPin;
use socket_patch_core::patch::rollback::{
cannot_rollback_error, rollback_package_patch, verify_file_rollback, RollbackResult,
VerifyRollbackResult, VerifyRollbackStatus,
};
use socket_patch_core::telemetry::{track_patch_rollback_failed, track_patch_rolled_back};
use socket_patch_core::utils::purl::{patch_matches, strip_purl_qualifiers};
-use socket_patch_core::patch::redirect::upstream::HostedPin;
use socket_patch_core::vendor::{purl_keys_cover, RevertOpts, VendorState};
use std::collections::{HashMap, HashSet};
use std::path::{Path, PathBuf};
@@ -1026,7 +1026,8 @@
.iter()
.map(|(code, detail)| (code.to_string(), detail.clone())),
);
- out.edited_files.extend(outcome.reverted_files.iter().cloned());
+ out.edited_files
+ .extend(outcome.reverted_files.iter().cloned());
let unwound: Vec<_> = vlt_targets
.into_iter()
.filter(|t| out.reverted.iter().any(|p| p == &t.purl))
@@ -1170,7 +1171,11 @@
} else if !args.common.silent {
println!(
"{} the pre-v5 hosted ledger {}: no lockfile pins a hosted patch.",
- if args.common.dry_run { "Would remove" } else { "Removed" },
+ if args.common.dry_run {
+ "Would remove"
+ } else {
+ "Removed"
+ },
socket_patch_core::patch::redirect::REDIRECT_STATE_REL
);
}
diff --git a/crates/socket-patch-cli/src/commands/scan/discovery.rs b/crates/socket-patch-cli/src/commands/scan/discovery.rs
--- a/crates/socket-patch-cli/src/commands/scan/discovery.rs
+++ b/crates/socket-patch-cli/src/commands/scan/discovery.rs
@@ -168,29 +168,32 @@
}
// `(ledger key, base purl, entry)`; the artifact fallback has no
// entries to probe, so it never reports unwired keys.
- let candidates: Vec<(String, String, Option<&socket_patch_core::vendor::VendorEntry>)> =
- match state {
- Ok(state) => state
- .entries
- .iter()
- .map(|(key, entry)| {
- (
- key.clone(),
- strip_purl_qualifiers(&entry.base_purl).to_string(),
- Some(entry),
- )
- })
- .collect(),
- // Corrupt/unreadable ledger (a MISSING file is Ok(empty) above):
- // recover the vendored set from the committed artifacts, or
- // `scan --prune` (whose ledger exemption also degrades to empty)
- // would delete still-vendored packages' manifest entries and blobs.
- Err(_) => vendored_purls_from_artifacts(common)
- .await
- .into_iter()
- .map(|base| (base.clone(), base, None))
- .collect(),
- };
+ let candidates: Vec<(
+ String,
+ String,
+ Option<&socket_patch_core::vendor::VendorEntry>,
+ )> = match state {
+ Ok(state) => state
+ .entries
+ .iter()
+ .map(|(key, entry)| {
+ (
+ key.clone(),
+ strip_purl_qualifiers(&entry.base_purl).to_string(),
+ Some(entry),
+ )
+ })
+ .collect(),
+ // Corrupt/unreadable ledger (a MISSING file is Ok(empty) above):
+ // recover the vendored set from the committed artifacts, or
+ // `scan --prune` (whose ledger exemption also degrades to empty)
+ // would delete still-vendored packages' manifest entries and blobs.
+ Err(_) => vendored_purls_from_artifacts(common)
+ .await
+ .into_iter()
+ .map(|base| (base.clone(), base, None))
+ .collect(),
+ };
// Composer by release identity: a ledger `@3.0.2.0` is the crawled
// `@3.0.2`, not a second package to supplement.
let key = |p: &str| composer_purl_identity(p).unwrap_or_else(|| normalize_purl(p).into_owned());
@@ -1038,7 +1041,9 @@
..GlobalArgs::default()
};
let state = socket_patch_core::vendor::load_state(root).await;
- vendored_ledger_supplement(&args, crawled, &state).await.packages
+ vendored_ledger_supplement(&args, crawled, &state)
+ .await
+ .packages
}
/// A ledger entry vendored as `@3.0.2.0` is the crawled composer
@@ -1073,7 +1078,9 @@
out.iter().map(|p| &p.purl).collect::<Vec<_>>()
);
- let out = vendored_ledger_supplement(&args, &[], &Ok(state)).await.packages;
+ let out = vendored_ledger_supplement(&args, &[], &Ok(state))
+ .await
+ .packages;
assert_eq!(
out.iter().map(|p| p.purl.as_str()).collect::<Vec<_>>(),
vec!["pkg:composer/psr/log@3.0.2.0"]
@@ -1176,7 +1183,10 @@
let state = npm_ledger_with_lock(tmp.path(), lock.as_deref()).await;
let out = vendored_ledger_supplement(&args, &[], &state).await;
assert_eq!(
- out.packages.iter().map(|p| p.purl.as_str()).collect::<Vec<_>>(),
+ out.packages
+ .iter()
+ .map(|p| p.purl.as_str())
+ .collect::<Vec<_>>(),
vec!["pkg:npm/left-pad@1.3.0"],
"lock={lock:?}"
);
diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs
--- a/crates/socket-patch-cli/src/commands/scan/hosted.rs
+++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs
@@ -789,6 +789,43 @@
// ownership known" for both consumers.
let mut vendor_state = socket_patch_core::vendor::load_state(&common.cwd).await;
+ // Read candidate files BEFORE the takeover to detect encoding issues
+ // early. The encoding check must happen before any writes (see the
+ // pre-takeover encoding guard below).
+ let read = if !candidates.is_empty() {
+ engine::read_candidate_files(&view, &std::collections::BTreeSet::new(), &candidates).await
+ } else {
+ CandidateFiles::default()
+ };
+
+ // PRE-TAKEOVER ENCODING GUARD: refuse if any undecodable candidate file
+ // matches a takeover-capable candidate's ecosystem. This prevents the
+ // takeover from writing before the encoding check in `engine::guard`
+ // would refuse the run, ensuring "nothing was written" stays true.
+ if !read.undecodable_reads.is_empty() {
+ use std::collections::BTreeSet;
+ let takeover_capable = |p: &str| {
+ p.starts_with("pkg:cargo/")
+ || p.starts_with("pkg:npm/")
+ || p.starts_with("pkg:golang/")
+ || p.starts_with("pkg:pypi/")
+ };
+ let candidate_ecosystems: BTreeSet<&str> = candidates
+ .iter()
+ .filter(|c| takeover_capable(&c.purl))
+ .map(|c| c.dep.ecosystem.as_str())
+ .collect();
+ if let Some(rel) = read.undecodable_reads.iter().find(|rel| {
+ engine::file_ecosystem(rel).is_some_and(|eco| candidate_ecosystems.contains(eco))
+ }) {
+ return refuse(
+ common,
+ scan_result.take(),
+ &engine::undecodable_refusal(rel),
+ );
+ }
+ }
+
// Cross-mode takeover of still-vendored purls (see `vendored_takeover`).
let Takeover {
pre_warnings: takeover_pre_warnings,
@@ -802,15 +839,13 @@
Err(refusal) => return refuse(common, scan_result.take(), &refusal),
};
- // Read the project's candidate files. Skipped when no candidate
- // survived and no dry-run takeover preview is pending (the rewriters do
- // nothing without a dep); everything after the rewrite still runs. A
- // dry-run takeover preview still needs the root locks for the
- // install-policy previews below.
- let read = if !candidates.is_empty() || !dry_run_takeover_urls.is_empty() {
+ // Re-read candidate files if the takeover modified any, or if a dry-run
+ // takeover preview is pending (the rewriters need the root locks for
+ // install-policy previews). Otherwise reuse the pre-takeover read.
+ let read = if !takeover_files.is_empty() || !dry_run_takeover_urls.is_empty() {
engine::read_candidate_files(&view, &std::collections::BTreeSet::new(), &candidates).await
} else {
- CandidateFiles::default()
+ read
};
let mut python_metadata = std::collections::BTreeMap::new();
@@ -932,7 +967,8 @@
socket_patch_core::utils::fs::read_regular_to_string_sync(path).ok()
})
};
- let rewrite_options = || RewriteOptions {
+ let rewrite_options = || {
+ RewriteOptions {
dry_run: common.dry_run,
targets_pipenv_lock,
pipenv_major,
@@ -944,6 +980,7 @@
npm_allow_remote_config: !common.no_npm_allow_remote_config,
npm_outer: &npm_outer,
blocking: true,
+ }
};
// The rollout gate plans again without its deferred rows: keep what
// the second pass needs.
@@ -2304,13 +2341,19 @@
/// artifacts, then verify with `vex`. After a vendored→hosted takeover
/// (`vendored_removed`) the commit also has to carry the deleted vendored
/// ledger entries and artifacts.
-fn format_next_steps(files: &[String], edits: &[socket_patch_core::patch::redirect::FileEdit], vendored_removed: bool) -> Vec<String> {
+fn format_next_steps(
+ files: &[String],
+ edits: &[socket_patch_core::patch::redirect::FileEdit],
+ vendored_removed: bool,
+) -> Vec<String> {
if files.is_empty() && !vendored_removed {
return Vec::new();
}
let mut commit: Vec<String> = Vec::new();
if vendored_removed {
- commit.push(".socket/vendor/ (the removed vendored ledger entries and artifacts)".to_string());
+ commit.push(
+ ".socket/vendor/ (the removed vendored ledger entries and artifacts)".to_string(),
+ );
}
commit.extend(files.iter().cloned());
let npm = files
@@ -4391,19 +4434,43 @@
use super::npm_allow_remote_one_line;
let hosts = ["patch.socket.dev"];
let cases = [
- (npm_allow_remote_configured_detail(&hosts, true, false), "Note: set"),
- (npm_allow_remote_configured_detail(&hosts, false, false), "Note: set"),
- (npm_allow_remote_configured_detail(&hosts, true, true), "Note: would set"),
- (npm_allow_remote_already_detail(&hosts), "Note: .npmrc already"),
- (npm_allow_remote_user_set_detail(&hosts, "none"), "Warning: npm >=12"),
- (npm_allow_remote_env_set_detail(&hosts, "npm_config_allow_remote", "none"), "Warning: npm >=12"),
+ (
+ npm_allow_remote_configured_detail(&hosts, true, false),
+ "Note: set",
+ ),
+ (
+ npm_allow_remote_configured_detail(&hosts, false, false),
+ "Note: set",
+ ),
+ (
+ npm_allow_remote_configured_detail(&hosts, true, true),
+ "Note: would set",
+ ),
+ (
+ npm_allow_remote_already_detail(&hosts),
+ "Note: .npmrc already",
+ ),
+ (
+ npm_allow_remote_user_set_detail(&hosts, "none"),
+ "Warning: npm >=12",
+ ),
+ (
+ npm_allow_remote_env_set_detail(&hosts, "npm_config_allow_remote", "none"),
+ "Warning: npm >=12",
+ ),
(npm_allow_remote_manual_detail(&hosts), "Warning: npm >=12"),
- (npm_allow_remote_unreadable_detail(&hosts, "is a symlink"), "Warning: npm >=12"),
+ (
+ npm_allow_remote_unreadable_detail(&hosts, "is a symlink"),
+ "Warning: npm >=12",
+ ),
];
for (detail, start) in cases {
let line = npm_allow_remote_one_line(&detail);
assert!(line.starts_with(start), "{line}");
- assert!(!line.contains('\n') && line.ends_with("(details: --verbose)."), "{line}");
+ assert!(
+ !line.contains('\n') && line.ends_with("(details: --verbose)."),
+ "{line}"
+ );
}
}
}
diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs
--- a/crates/socket-patch-cli/src/commands/scan/mod.rs
+++ b/crates/socket-patch-cli/src/commands/scan/mod.rs
@@ -35,17 +35,17 @@
use super::get::{download_and_apply_patches_with, DownloadParams, DownloadRun};
+use self::policy::{load_invocation_policy, InvocationPolicy, PolicyLoadError, ScanPolicy};
pub use self::socket_yml_args::{SocketYmlArgs, MIN_SEVERITY_ENV};
-use self::policy::{load_invocation_policy, InvocationPolicy, PolicyLoadError, ScanPolicy};
mod discovery;
mod gc;
pub(crate) mod hosted;
pub(crate) mod policy;
-mod socket_yml_args;
pub(crate) mod render;
pub(crate) mod rollout;
pub mod rollout_args;
+mod socket_yml_args;
pub(crate) mod vendor_flow;
use self::discovery::{
@@ -65,13 +65,13 @@
pub(crate) use self::hosted::boxed_run_redirect_selected;
use self::hosted::run_redirect;
pub(crate) use self::hosted::{vlt_rollback_heal, vlt_takeover_heal};
-pub(crate) use self::vendor_flow::{
- boxed_vendor_step, preview_vendor_json, print_dry_run_refusals, VendorStep,
-};
use self::vendor_flow::{
boxed_vendor_interactive_path, boxed_vendor_json_path, fold_vendored_skips_into_apply,
partition_skipped_selected,
};
+pub(crate) use self::vendor_flow::{
+ boxed_vendor_step, preview_vendor_json, print_dry_run_refusals, VendorStep,
+};
/// Packages per batch request on the authenticated API when `--batch-size`
/// is not given: the server's own per-request maximum
@@ -318,11 +318,7 @@
/// `requests`), or a purl with or without its version
/// (`pkg:npm/lodash`, `pkg:pypi/requests@2.31.0`). Repeat the flag or
/// separate with commas
- #[arg(
- long = "package",
- env = "SOCKET_SCAN_PACKAGES",
- value_delimiter = ','
- )]
+ #[arg(long = "package", env = "SOCKET_SCAN_PACKAGES", value_delimiter = ',')]
pub packages: Vec<String>,
/// On a successful scan, also generate an OpenVEX 0.2.0 document.
@@ -500,9 +496,10 @@
telemetry.flush().await;
let error_count = failures.len();
if error_count > 0 && error_count == packages.len() {
- let err = failures
- .last()
- .map_or_else(|| "all patch-detail queries failed".to_string(), |(_, e)| e.clone());
+ let err = failures.last().map_or_else(
+ || "all patch-detail queries failed".to_string(),
+ |(_, e)| e.clone(),
+ );
let message = format!("all {error_count} patch-detail queries failed: {err}");
if detail_error_line {
eprintln!("{}", render::fetch_details_failed(&failures));
@@ -568,7 +565,11 @@
packages: &[BatchPackagePatches],
result: Option<&mut serde_json::Value>,
) -> Vec<rollout::Row> {
- let failed: Vec<String> = discovered.failed.iter().map(|(purl, _)| purl.clone()).collect();
+ let failed: Vec<String> = discovered
+ .failed
+ .iter()
+ .map(|(purl, _)| purl.clone())
+ .collect();
stage.incomplete = rollout::lookup_incomplete(&recorded.index, &failed, batch_failed);
let rows = rollout::classify(&discovered.offers, &recorded.index, &stage.project);
if let Some(result) = result {
@@ -1317,7 +1318,8 @@
let joined = cwd.join(raw);
if raw.contains(['*', '?', '[']) {
let pattern = joined.to_string_lossy().into_owned();
- let matches = glob::glob(&pattern).map_err(|e| format!("invalid path pattern `{raw}`: {e}"))?;
+ let matches =
+ glob::glob(&pattern).map_err(|e| format!("invalid path pattern `{raw}`: {e}"))?;
let before = dirs.len();
dirs.extend(
matches
@@ -1390,7 +1392,10 @@
}
// One budget per invocation (§5.2): the directories spend it in sorted
// order, and a package admitted in one is admitted free in the next.
- let configured = match args.rollout.resolve_from_env(invocation.policy.max_new_patches()) {
+ let configured = match args
+ .rollout
+ .resolve_from_env(invocation.policy.max_new_patches())
+ {
Ok(max) => max,
Err(message) => {
eprintln!("Error: {message}");
@@ -1491,7 +1496,10 @@
// error.
let configured_cap = match args.rollout.carry.as_ref() {
Some(carry) => carry.lock().configured,
- None => match args.rollout.resolve_from_env(invocation.policy.max_new_patches()) {
+ None => match args
+ .rollout
+ .resolve_from_env(invocation.policy.max_new_patches())
+ {
Ok(max) => max,
Err(message) => {
eprintln!("Error: {message}");
@@ -1499,11 +1507,8 @@
}
},
};
- let mut stage = rollout::Stage::new(
- configured_cap,
- args.rollout.carry.clone(),
- &args.common.cwd,
- );
+ let mut stage =
+ rollout::Stage::new(configured_cap, args.rollout.carry.clone(), &args.common.cwd);
// Strict airgap (CLI_CONTRACT.md `--offline`): scan's patch discovery
// is remote data, so refuse before the crawl and before the API client
@@ -1704,8 +1709,11 @@
.filter(|pkg| args.common.purl_ecosystem_selected(&pkg.purl))
.collect();
- let package_specs: Vec<&String> =
- args.packages.iter().filter(|s| !s.trim().is_empty()).collect();
+ let package_specs: Vec<&String> = args
+ .packages
+ .iter()
+ .filter(|s| !s.trim().is_empty())
+ .collect();
let filtered_crawled: Vec<_> = if package_specs.is_empty() {
filtered_crawled
} else {
@@ -1860,13 +1868,12 @@
// `redirectState` rides the empty-discovery envelope too
// (same rule as the ≥1-package path). `wiringLive` is empty
// by construction: this run covered zero packages.
- let redirect_state = (!args.common.is_global()).then_some(
- crate::commands::hosted_state_from_pins(
+ let redirect_state =
+ (!args.common.is_global()).then_some(crate::commands::hosted_state_from_pins(
&socket_patch_core::patch::redirect::upstream::HostedPin::all(
ctx.discovery().await,
),
- ),
- );
+ ));
if let Some(state) = redirect_state_json(redirect_state.as_ref(), &[]) {
result["redirectState"] = state;
}
@@ -2222,7 +2229,8 @@
// A report-only run selects nothing, but a severity floor or
// `enabled: false` still hides candidates; report them like the
// human arm does (the detail fetch runs only then).
- if !apply && !vendor && policy.reports_selection() && !all_packages_with_patches.is_empty() {
+ if !apply && !vendor && policy.reports_selection() && !all_packages_with_patches.is_empty()
+ {
if let Err((code, message)) = discover_selected(
&api_client,
&all_packages_with_patches,
@@ -2515,12 +2523,7 @@
&all_packages_with_patches,
None,
);
- updates = offer_updates(
- &rows,
- &discovered,
- &recorded,
- &all_packages_with_patches,
- );
+ updates = offer_updates(&rows, &discovered, &recorded, &all_packages_with_patches);
rows
}
// `discover_selected` already printed the failure to stderr.
@@ -2982,14 +2985,20 @@
dirs.iter()
.map(|(d, explicit)| {
(
- d.strip_prefix(tmp.path()).unwrap().to_string_lossy().replace('\\', "/"),
+ d.strip_prefix(tmp.path())
+ .unwrap()
+ .to_string_lossy()
+ .replace('\\', "/"),
*explicit,
)
})
.collect()
};
- let got = project_dirs(tmp.path(), &["apps/*".into(), "libs/core".into(), "apps/web".into()])
- .unwrap();
+ let got = project_dirs(
+ tmp.path(),
+ &["apps/*".into(), "libs/core".into(), "apps/web".into()],
+ )
+ .unwrap();
// Named literally = explicit (also when a glob matches it too).
assert_eq!(
rel(got),
diff --git a/crates/socket-patch-cli/src/commands/scan/policy.rs b/crates/socket-patch-cli/src/commands/scan/policy.rs
--- a/crates/socket-patch-cli/src/commands/scan/policy.rs
+++ b/crates/socket-patch-cli/src/commands/scan/policy.rs
@@ -11,9 +11,9 @@
use socket_patch_core::api::types::PatchSearchResult;
use socket_patch_core::manifest::schema::PatchManifest;
use socket_patch_core::policy::{
- canon, find_repo_root_with_warnings, policy_block, FilteredEntry, RetainedEntry, patch_severity_order, repo_relative_checked, sanitize, severity_name,
- DiskPolicyFs, FilterReason, Offers, PolicyError, PolicySource, PolicyWarning, Root, SelectionPolicy,
- PATCHES_DISABLED,
+ canon, find_repo_root_with_warnings, patch_severity_order, policy_block, repo_relative_checked,
+ sanitize, severity_name, DiskPolicyFs, FilterReason, FilteredEntry, Offers, PolicyError,
+ PolicySource, PolicyWarning, RetainedEntry, Root, SelectionPolicy, PATCHES_DISABLED,
};
use socket_patch_core::utils::purl::normalize_purl;
@@ -42,12 +42,18 @@
/// Load the policy for `args` (4.5): `--global` scans have no repo and read
/// no file; everything else reads the repo root's socket.yml.
pub(crate) fn load_invocation_policy(args: &ScanArgs) -> Result<InvocationPolicy, PolicyLoadError> {
- let overrides = args.socket_yml.overrides().map_err(PolicyLoadError::Usage)?;
+ let overrides = args
+ .socket_yml
+ .overrides()
+ .map_err(PolicyLoadError::Usage)?;
let cwd = std::fs::canonicalize(&args.common.cwd).unwrap_or_else(|_| args.common.cwd.clone());
if args.common.is_global() {
- let policy = SelectionPolicy::load(&socket_patch_core::policy::MemoryPolicyFs::default(), &overrides)
- .map_err(PolicyLoadError::Policy)?
- .0;
+ let policy = SelectionPolicy::load(
+ &socket_patch_core::policy::MemoryPolicyFs::default(),
+ &overrides,
+ )
+ .map_err(PolicyLoadError::Policy)?
+ .0;
return Ok(InvocationPolicy {
policy,
repo_root: cwd,
@@ -56,8 +62,8 @@
});
}
let (repo_root, mut warnings) = find_repo_root_with_warnings(&cwd);
- let (policy, load_warnings) =
- SelectionPolicy::load(&DiskPolicyFs::new(&repo_root), &overrides).map_err(PolicyLoadError::Policy)?;
+ let (policy, load_warnings) = SelectionPolicy::load(&DiskPolicyFs::new(&repo_root), &overrides)
+ .map_err(PolicyLoadError::Policy)?;
warnings.extend(load_warnings);
Ok(InvocationPolicy {
policy,
@@ -138,7 +144,12 @@
impl ScanPolicy {
/// The policy for the project rooted at `root_dir`.
- pub(crate) fn for_root(invocation: &InvocationPolicy, root_dir: &Path, explicit: bool, global: bool) -> Self {
+ pub(crate) fn for_root(
+ invocation: &InvocationPolicy,
+ root_dir: &Path,
+ explicit: bool,
+ global: bool,
+ ) -> Self {
let root_dir = std::fs::canonicalize(root_dir).unwrap_or_else(|_| root_dir.to_path_buf());
let project = repo_relative_checked(&invocation.repo_root, &root_dir).unwrap_or_default();
let root_verdict = if global {
@@ -171,7 +182,9 @@
severity: None,
});
}
- let announce_warnings = !invocation.warned.swap(true, std::sync::atomic::Ordering::Relaxed);
+ let announce_warnings = !invocation
+ .warned
+ .swap(true, std::sync::atomic::Ordering::Relaxed);
Self {
policy: invocation.policy.clone(),
warnings,
@@ -224,7 +237,10 @@
/// exclude stays in the query (so `upgradeAvailable` can be reported)
/// but joins the retained set, which never reaches a writer.
pub(crate) fn admit_crawled(&self, purl: &str) -> bool {
- let verdict = self.root_verdict.clone().and_then(|()| self.policy.admits_purl(purl));
+ let verdict = self
+ .root_verdict
+ .clone()
+ .and_then(|()| self.policy.admits_purl(purl));
let reason = match verdict {
Ok(()) => return true,
Err(reason) => reason,
@@ -334,7 +350,8 @@
// (not when a lower-ranked admitted patch simply wins).
let top_withheld = self.policy.admits_severity(patch_severity_order(&group[0]));
if let Err(reason) = top_withheld {
- let upgrade_withheld = chosen.is_some() && chosen == recorded_at && recorded_at != Some(0);
+ let upgrade_withheld =
+ chosen.is_some() && chosen == recorded_at && recorded_at != Some(0);
if chosen.is_none() || upgrade_withheld {
report.filtered.push(FilteredEntry {
purl: Some(canon(&purl)),
@@ -522,17 +539,20 @@
let verdict = if !policy.enabled() {
Err(FilterReason::Disabled)
} else {
- root_verdict.clone().and_then(|()| policy.admits_purl(purl)).and_then(|()| {
- // The floor only hides a package when none of its patches pass.
- match group
- .iter()
- .map(|p| policy.admits_severity(patch_severity_order(p)))
- .find(Result::is_ok)
- {
- Some(ok) => ok,
- None => policy.admits_severity(patch_severity_order(group[0])),
- }
- })
+ root_verdict
+ .clone()
+ .and_then(|()| policy.admits_purl(purl))
+ .and_then(|()| {
+ // The floor only hides a package when none of its patches pass.
+ match group
+ .iter()
+ .map(|p| policy.admits_severity(patch_severity_order(p)))
+ .find(Result::is_ok)
+ {
+ Some(ok) => ok,
+ None => policy.admits_severity(patch_severity_order(group[0])),
+ }
+ })
};
if let Err(reason) = verdict {
out.push((
diff --git a/crates/socket-patch-cli/src/commands/scan/render.rs b/crates/socket-patch-cli/src/commands/scan/render.rs
--- a/crates/socket-patch-cli/src/commands/scan/render.rs
+++ b/crates/socket-patch-cli/src/commands/scan/render.rs
@@ -746,7 +746,10 @@
#[test]
fn report_only_hint_names_agent_mode() {
- assert_eq!(report_only_hint()[0], "To apply these patches in place, run:");
+ assert_eq!(
+ report_only_hint()[0],
+ "To apply these patches in place, run:"
+ );
assert!(report_only_hint()[1].contains("--mode agent"));
}
diff --git a/crates/socket-patch-cli/src/commands/scan/rollout.rs b/crates/socket-patch-cli/src/commands/scan/rollout.rs
--- a/crates/socket-patch-cli/src/commands/scan/rollout.rs
+++ b/crates/socket-patch-cli/src/commands/scan/rollout.rs
@@ -4,8 +4,10 @@
use std::collections::{BTreeMap, BTreeSet, HashSet};
-use socket_patch_core::rollout::{canonical_base_purl, severity_label, MaxNew, MaxNewSource, Recorded, RolloutPlan};
pub(crate) use socket_patch_core::rollout::stage::*;
+use socket_patch_core::rollout::{
+ canonical_base_purl, severity_label, MaxNew, MaxNewSource, Recorded, RolloutPlan,
+};
... diff truncated: showing 800 of 6990 linesYou can send follow-ups to the cloud agent here. |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
Collaborator
Author
|
Ready for review. Head
Generated by Claude Code |
Tanmay Singla (Tanmay182003)
approved these changes
Oct 5, 2026
Mikola Lysenko (mikolalysenko)
removed
the
Ready for review
CLI_CONTRACT.md: kept this PR's #721 non-UTF-8 candidate paragraph and main's expanded gem stale-install (global bundler config tier) paragraph. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
bugbot run Generated by Claude Code |
Collaborator
Author
|
[agent] Three checks are red on Generated by Claude Code |
Brings in the vex_consumed alias test fix (#849) that main's red test/test-release/coverage jobs were waiting on. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3e79ecb. Configure here.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LLM Description written by Claude Code:claude-opus-5-5
Fixes #721
Summary
Windows PowerShell 5.1 writes
pip freeze > requirements.txtas UTF-16 LE with a BOM, and pip installs from it. socket-patch's hosted and lock-only paths read the file as UTF-8 only and treated a decode failure as "file absent":status: success,redirected: 0and no warning, so pip kept installing the unpatched release.Root cause
CandidateFiles::read(crates/socket-patch-core/src/hosted/engine.rs) ran disk reads throughview.read_text(rel).await.ok(), so anInvalidData(non-UTF-8) error looked exactly like a missing file. The in-memory branch did the same on purpose, to stay at parity with disk. Lock-only discovery (requirements_treeinvendor/lock_inventory/pypi.rs) also used a strict UTF-8read_text(..).ok().Fix
undecodable_reads.engine::guardrefuses the run with the existingcandidate_file_unreadablecode when a candidate of that file's ecosystem could rewrite it. The message names the file and the remedy (re-save it as UTF-8). Exit 1, nothing written,--dry-runincluded. Other ecosystems' runs aren't affected. This also protects files such as a UTF-16nuget.config, which the rewriters would otherwise have treated as missing.utils::requirements::decodemirrors pip'sauto_decodeBOM table (UTF-16 LE/BE, UTF-32 BE, otherwise UTF-8, in pip's order).requirements_treeuses it for the root file and every in-root-rinclude, so the pins are discovered. Discovery is read-only, so decoding here is safe. The hosted rewrite then refuses loudly as described above.vendored_takeovernow runs the same rule (engine::undecodable_guard) before any revert, wet and--dry-runalike, so a refusal never strands a reverted purl.-rinclude by name, with the re-save hint. Before, a UTF-16 root got a bare "cannot read", and a UTF-16 include that pip installs from was silently skipped.I chose refusal over writing UTF-16 back. The rewriters, the restore snapshots and the rollback paths all work on UTF-8 text. A fail-closed refusal that names the file matches the in-memory engine's existing
candidate_file_unreadablerule and the contract'slockfile_unreadabledefinition ("non-UTF-8"). If maintainers want byte-faithful UTF-16 rewrites, that can be a follow-up.The wrappers (
npm/,pypi/,gem/) only dispatch to the binary, so they need no change.Tests (red → green)
in_process_get_hosted_ecosystems::pypi_requirements_hosted_refuses_a_utf16_file(UTF-16 LE and BE)left: 0, right: 1)scan_requirements_lock_only::lock_only_scan_discovers_utf16_pins(LE and BE, hosted and--vendor)lockfileOnlyPackages: 0hosted::engine::tests::an_undecodable_candidate_file_refuses_its_ecosystem-rinclude, disk + memorylock_inventory::tests::requirements_utf16_files_are_inventoriedmode_migration_pypi::undecodable_candidate_refuses_before_the_takeover_reverts(wet +--dry-run)left: 0, right: 1)pypi_requirements::tests::a_utf16_requirements_file_is_refused_by_nameutils::requirements::tests::decode_follows_pips_byte_order_marksLocal checks:
cargo clippy --workspace --all-features -- -D warnings: clean.cargo fmt: my hunks are rustfmt-clean.mainitself isn't fmt-clean under the pinned 1.93.1 toolchain, and CI doesn't gate on fmt, so I didn't reformat unrelated files.cargo test -p socket-patch-core --all-features: 4845 passed. 4 failed, all read-only-permission tests that can't fail when run as root (uid 0, this sandbox):copy_tree::relax_loop_must_not_traverse_symlinked_root,vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry,pypi_poetry::wire_write_failure_maps_error_and_leaves_lock_untouched,pypi_requirements::wire_failure_rolls_back_already_written_files. None touch the changed code.--lib(834),hosted_memory_engine,hosted_memory_parity,hosted_memory_rollout,covgap_commands_scan_hosted,e2e_vex_redirect,in_process_redirect_pipenv,in_process_rollback_hosted,mode_migration_pypi,scan_requirements_lock_onlyandin_process_get_hosted_ecosystemsall pass.in_process_redirect: 104 passed. 3 failed, againchmod 0o555write-failure tests that root bypasses.b3daafc: all green (479 success, 6 skipped). Onenative (macos-latest, 1.3.10)Bun job against the production patch hosts failed on the first attempt. It passed on5444dd6, and this PR touches no Bun code. It passed on its single re-run.Note
Medium Risk
Changes hosted scan/get failure modes (exit 1 vs silent success) and lock-only discovery for Windows-style requirements files; takeover ordering avoids partial vendored state on refusal.
Overview
Fixes #721: UTF-16
requirements.txt(common from Windowspip freeze) was treated as missing, so hosted runs could exit 0 still unpatched and lock-only scan reported no packages.Discovery now decodes requirements files like pip (
decodeon BOM: UTF-16/UTF-32, else UTF-8), so lock-only scan finds pins in UTF-16 roots and-rincludes.Hosted rewrites record non-UTF-8 candidate files in
undecodable_readsand fail closed withcandidate_file_unreadable(exit 1, including--dry-run) when that ecosystem would edit the file—rewriters stay UTF-8-only. The vendored→hosted takeover runs the same check before any revert so a refusal cannot leave reverted wiring unpatched.Vendored mode refuses non-UTF-8 requirements roots/includes with
pypi_no_requirementsinstead of skipping them.CLI_CONTRACT.mddocuments the behavior.Reviewed by Cursor Bugbot for commit 3e79ecb. Configure here.
Generated by Claude Code