Repository navigation
Fix UTF-16 requirements.txt silently skipped (#721) - #724
Open
Mikola Lysenko (mikolalysenko) wants to merge 14 commits into
Open
Mikola Lysenko (mikolalysenko) wants to merge 14 commits into
Mikola Lysenko (mikolalysenko) wants to merge 14 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 |
Collaborator
Author
|
[agent]
The case logs are in artifact Update: the single re-run of Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
Collaborator
Author
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
removed
the
Ready for review
Resolved crates/socket-patch-core/src/hosted/engine.rs: this branch adds CandidateFiles::undecodable_reads (#721) and main added CandidateFiles::gradle_unreadable (#646) in the same spot of the struct and of the destructuring in rewrite(). Kept both fields; the two are independent (a non-UTF-8 Gradle file in a memory view is recorded in both, and each guard refuses on its own). Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
bugbot run Generated by Claude Code |
Resolves the CLI_CONTRACT.md conflict by keeping both the non-UTF-8 candidate files paragraph and main's updated gem stale-install guard paragraph. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YED4tY7Yytk79MTPzLnSfA
main has failed socket-patch-core's lib tests since Gradle support (#646) and the digest helpers (#865) both landed. The guard test production_digests_go_through_the_helpers flags three files #646 added that still hash inline: crawlers/gradle_cache.rs, patch/jvm_jar.rs and patch/sidecars/maven.rs. That breaks test, test-release and coverage on every open PR. Each inline sha1/sha256 call now goes through sha1_hex_of or sha256_hex_of, which compute the same lowercase hex. Behaviour is unchanged. Assisted-by: Claude Code:claude-opus-5-5 (cherry picked from commit 659ac2c)
Collaborator
Author
|
[agent] Pushed Generated by Claude Code |
Collaborator
Author
|
bugbot run Generated by Claude Code |
Collaborator
Author
|
[agent] On Generated by Claude Code |
A non-UTF-8 settings.gradle was recorded both as an undecodable candidate (#721) and in gradle_unreadable. The run-wide #721 guard then refused the whole hosted run with candidate_file_unreadable (exit 1), pre-empting the Gradle planner's own per-build refusal (redirect_gradle_build_file_unreadable, exit 0) that main added. Files the Gradle planner already refuses are now dropped from undecodable_reads, so only that build is refused and the run goes ahead. Refs #721 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YED4tY7Yytk79MTPzLnSfA
Collaborator
Author
|
[agent] Pushed Generated by Claude Code |
Collaborator
Author
|
bugbot run Generated by Claude Code |
Collaborator
Author
|
[agent] On Generated by Claude Code |
Collaborator
Author
|
[agent]
Case logs are in artifact Generated by Claude Code |
Collaborator
Author
|
[agent] Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
Collaborator
Author
|
[agent] Ready for review at head
Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
removed
the
Ready for review
Main replaced gem_manifest_unsupported with gem_refusal (Bundler mirror gate, #681) and added env-aware include tests (#994). Keep main's refusal plumbing while still dropping undecodable gem reads, and layer the bytes helper this branch added for UTF-16 fixtures under main's env-aware helper so both test sets share one driver. Co-Authored-By: Claude <noreply@anthropic.com>
Picks up #1016's digest pending-list fix so test/coverage aren't red from main. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
bugbot run Generated by Claude Code |
The run-wide non-UTF-8 check (#721) pre-empted two refusals main owns: - An unreadable socket-patch.sbt was refused as candidate_file_unreadable instead of redirect_sbt_owned_file_unreadable, failing e2e_sbt_hosted::hosted_unreadable_owned_file_is_refused_untouched. - With no readable Gradle build, a stray non-UTF-8 Gradle file (a lone settings.gradle or a gradle.lockfile) refused a whole Maven run, pom.xml included, although the Gradle planner never runs and never rewrites it (Bugbot). On disk, socket-patch.sbt is now left to the sbt refusal, and with no readable Gradle build, Gradle-owned files no longer refuse the run. Refs #721 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YED4tY7Yytk79MTPzLnSfA
Collaborator
Author
|
[agent] Pushed
Checked locally: both the sbt test and the new engine test fail without the fix and pass with it. Generated by Claude Code |
Collaborator
Author
|
bugbot run Generated by Claude Code |
92a44fd dropped socket-patch.sbt from the run-wide non-UTF-8 check so the sbt refusal could report its own code. That also hid it from the pre-takeover check, so a mixed run (a vendored PyPI purl plus sbt) could revert vendored wiring and only then be refused, leaving those packages unpatched in both modes (Bugbot). The file stays in the early check, which now refuses it with redirect_sbt_owned_file_unreadable, before any revert. Refs #721 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YED4tY7Yytk79MTPzLnSfA
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 950b9e6. 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 redirect guards and vendored takeover ordering so mis-encoded lock/requirements files fail loudly instead of silently skipping patches; discovery decode affects which packages reach the patch API.
Overview
Fixes #721: non-UTF-8 candidate files (especially UTF-16
requirements.txtfrom Windowspip freeze) are no longer treated as missing, which previously let hosted runs exit 0 unpatched and lock-only scan report no packages.Hosted mode records undecodable candidates in
undecodable_readsand fails closed viaundecodable_guard/candidate_file_unreadable(exit 1, no writes, including--dry-run). Vendored→hosted takeover runs the same check before any vendored revert so a refusal cannot leave reverted wiring unpatched. Gradle unreadable files and stray non-UTF-8 Gradle paths keep per-build or no-op behavior; unreadablesocket-patch.sbtkeepsredirect_sbt_owned_file_unreadable.Lock-only PyPI discovery decodes requirements files like pip (
utils::requirements::decodeBOM handling) so UTF-16 pins are found; vendored requirements wiring refuses non-UTF-8 root or-rincludes withpypi_no_requirements. CLI_CONTRACT.md documents the behavior.Tests cover hosted refusal, lock-only discovery, takeover ordering, vendored refusal, and pip BOM decoding.
Reviewed by Cursor Bugbot for commit 950b9e6. Configure here.
Generated by Claude Code