Skip to content

Commit ac1fdd6

Browse files
fix: second-pass bug sweep — crawler traversal guards, FIFO hangs, atomic writes, CLI contract (#111)
* fix: second-pass bug sweep across core and CLI + sweep tooling Fixes from the per-file review sweep (study-crates.ts / fix-bugs config), each landed with a regression test that failed on the old code: - crawlers: fail-closed coordinate guards on untrusted PURL components (deno/go/maven/npm/nuget/ruby) so `..`/absolute segments can't escape the package root during in-place apply; nuget legacy content-folder parse + empty NUGET_PACKAGES fallback; python purl subpath strip - patch engine: cargo sidecar checksum resync on rollback/remove, FIFO open hangs (O_NONBLOCK), copy_tree symlinked-root chmod, cow hardlink is_file gate, rollback/sidecar path-escape guards, create_dir_all perm-relax ordering, go_redirect reconcile + drive-letter escape - manifest/config writers: atomic write (stage+fsync+rename) for gem/composer/package.json/pyproject/go.mod sites; BOM tolerance - CLI: --silent honored in scan/list/get/remove/repair/setup; env-bool flags via parse_bool_flag; non-UTF-8 argv usage error; unlock lock-dir under --manifest-path; rollback go redirect backend + 4 CLI bugs; get UUID proxy fallback + blob-hash guard; output severity colors - vex: product.rs BOM/comment-header parsing, schema spec-optional fields; telemetry trailing-slash URL join Tooling: simplify.config.ts sweep prompt, fix-bugs prompt hardening, study-crates model export + no default timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(lock): drop self-defeating --break-lock advice on refused break-lock All five CI failures were the single RED test break_lock_refusal_does_not_advise_break_lock_again: a refused --break-lock (live holder) still printed "rerun with --break-lock". Replace emit()'s unused-path hint_dir param with a Hint enum so the break-probe refusal keeps only the `socket-patch unlock` pointer while the plain lock_held path keeps the full advice. Also post-merge reconciliation with #110: - rollback.rs tests: destructure rollback_patches' new 3-tuple (vendored_skipped) return - npm_crawler.rs tests: clippy cloned_ref_to_slice_refs cleanup Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 4488ed1 commit ac1fdd6

74 files changed

Lines changed: 7145 additions & 597 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ tar = "=0.4.46"
3535
flate2 = "=1.1.9"
3636
zip = { version = "=8.6.0", default-features = false, features = ["deflate"] }
3737
fs2 = "=0.4.3"
38+
libc = "=0.2.182"
3839
wiremock = "=0.6.5"
3940
portable-pty = "=0.9.0"
4041
testcontainers = "=0.27.3"

crates/socket-patch-cli/src/args.rs

Lines changed: 179 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -217,10 +217,10 @@ pub struct GlobalArgs {
217217
#[arg(long = "lock-timeout", env = "SOCKET_LOCK_TIMEOUT")]
218218
pub lock_timeout: Option<u64>,
219219

220-
/// Force-remove `<.socket>/apply.lock` before attempting
221-
/// acquisition. Use when you are certain no other socket-patch
222-
/// process is running (e.g. a previous run crashed in a way that
223-
/// stripped the OS lock but left the file). Emits a
220+
/// Reclaim a stale `<.socket>/apply.lock` left behind by a
221+
/// crashed run (the file is never deleted — deleting a lock file
222+
/// defeats mutual exclusion). Refuses with `lock_held` if a live
223+
/// socket-patch process still holds the lock. Emits a
224224
/// `lock_broken` warning event in the JSON envelope so the
225225
/// action is auditable. Only meaningful for mutating
226226
/// subcommands; other commands accept it silently.
@@ -282,10 +282,21 @@ impl GlobalArgs {
282282
}
283283

284284
/// Apply CLI-flag toggles for env-driven knobs by mirroring them into env
285-
/// vars. This is how `--debug` / `--no-telemetry` reach core code that
286-
/// reads `SOCKET_DEBUG` / `SOCKET_TELEMETRY_DISABLED` directly. Idempotent
287-
/// and a no-op when the flags are off.
285+
/// vars. This is how `--offline` / `--debug` / `--no-telemetry` reach core
286+
/// code that reads `SOCKET_OFFLINE` / `SOCKET_DEBUG` /
287+
/// `SOCKET_TELEMETRY_DISABLED` directly. Idempotent and a no-op when the
288+
/// flags are off.
289+
///
290+
/// `offline` matters most: the telemetry kill-switch
291+
/// (`socket_patch_core::utils::telemetry::is_telemetry_disabled`) honors the
292+
/// strict-airgap contract by reading `SOCKET_OFFLINE` from the env, so
293+
/// without this mirror a bare `--offline` flag (or a truthy spelling like
294+
/// `SOCKET_OFFLINE=yes` that core's `"1" | "true"` match doesn't recognize)
295+
/// still let telemetry fire a network request.
288296
pub fn apply_env_toggles(common: &GlobalArgs) {
297+
if common.offline {
298+
std::env::set_var("SOCKET_OFFLINE", "1");
299+
}
289300
if common.debug {
290301
std::env::set_var("SOCKET_DEBUG", "1");
291302
}
@@ -294,6 +305,53 @@ pub fn apply_env_toggles(common: &GlobalArgs) {
294305
}
295306
}
296307

308+
/// Every env var `GlobalArgs` binds (one per `env = "..."` attribute above).
309+
/// Single source of truth for [`scrub_empty_global_env_vars`] and the
310+
/// clean-environment test harnesses.
311+
pub const GLOBAL_ARG_ENV_VARS: &[&str] = &[
312+
"SOCKET_CWD",
313+
"SOCKET_MANIFEST_PATH",
314+
"SOCKET_API_URL",
315+
"SOCKET_API_TOKEN",
316+
"SOCKET_ORG_SLUG",
317+
"SOCKET_PROXY_URL",
318+
"SOCKET_ECOSYSTEMS",
319+
"SOCKET_DOWNLOAD_MODE",
320+
"SOCKET_OFFLINE",
321+
"SOCKET_GLOBAL",
322+
"SOCKET_GLOBAL_PREFIX",
323+
"SOCKET_JSON",
324+
"SOCKET_VERBOSE",
325+
"SOCKET_SILENT",
326+
"SOCKET_DRY_RUN",
327+
"SOCKET_YES",
328+
"SOCKET_LOCK_TIMEOUT",
329+
"SOCKET_BREAK_LOCK",
330+
"SOCKET_DEBUG",
331+
"SOCKET_TELEMETRY_DISABLED",
332+
];
333+
334+
/// Remove exported-but-**empty** `GlobalArgs` env vars before clap parses.
335+
///
336+
/// `SOCKET_CWD=` — the conventional shell/CI idiom for blanking a variable
337+
/// without unsetting it — must mean "unset, fall back to the default", not
338+
/// abort the command. [`parse_bool_flag`] already gives the bool flags that
339+
/// semantic, but clap rejects an empty `SOCKET_CWD` / `SOCKET_GLOBAL_PREFIX`
340+
/// ("a value is required"), `SOCKET_LOCK_TIMEOUT` ("cannot parse integer
341+
/// from empty string") and `SOCKET_ECOSYSTEMS` (the per-token validator)
342+
/// outright — a single stray blank var crashed every subcommand — and an
343+
/// empty `SOCKET_DOWNLOAD_MODE` / `SOCKET_MANIFEST_PATH` leaked `""` past
344+
/// the documented defaults. Called from `main` after legacy-name promotion
345+
/// and before clap runs. Only exactly-empty values are scrubbed; whitespace
346+
/// is significant in paths, so it is left for the parsers to judge.
347+
pub fn scrub_empty_global_env_vars() {
348+
for &var in GLOBAL_ARG_ENV_VARS {
349+
if matches!(std::env::var(var).as_deref(), Ok("")) {
350+
std::env::remove_var(var);
351+
}
352+
}
353+
}
354+
297355
impl Default for GlobalArgs {
298356
/// Defaults intended for **test struct literals** (e.g. `..GlobalArgs::default()`).
299357
///
@@ -350,28 +408,8 @@ mod tests {
350408

351409
/// Full list of env vars `GlobalArgs` reads, so each clap-parse test starts
352410
/// from a known-clean environment (no ambient `SOCKET_*` bleed-through).
353-
const SOCKET_ENV_VARS: &[&str] = &[
354-
"SOCKET_CWD",
355-
"SOCKET_MANIFEST_PATH",
356-
"SOCKET_API_URL",
357-
"SOCKET_API_TOKEN",
358-
"SOCKET_ORG_SLUG",
359-
"SOCKET_PROXY_URL",
360-
"SOCKET_ECOSYSTEMS",
361-
"SOCKET_DOWNLOAD_MODE",
362-
"SOCKET_OFFLINE",
363-
"SOCKET_GLOBAL",
364-
"SOCKET_GLOBAL_PREFIX",
365-
"SOCKET_JSON",
366-
"SOCKET_VERBOSE",
367-
"SOCKET_SILENT",
368-
"SOCKET_DRY_RUN",
369-
"SOCKET_YES",
370-
"SOCKET_LOCK_TIMEOUT",
371-
"SOCKET_BREAK_LOCK",
372-
"SOCKET_DEBUG",
373-
"SOCKET_TELEMETRY_DISABLED",
374-
];
411+
/// Aliases the production list so the scrub and the harness can't drift.
412+
const SOCKET_ENV_VARS: &[&str] = GLOBAL_ARG_ENV_VARS;
375413

376414
/// Snapshot/clear every `SOCKET_*` var, run `f`, then restore. Keeps the
377415
/// env-mutating clap tests hermetic and reversible.
@@ -392,6 +430,118 @@ mod tests {
392430
}
393431
}
394432

433+
/// Clear the extra env the core telemetry gate reads beyond the
434+
/// `SOCKET_*` set (`is_telemetry_disabled` also consults `VITEST` and the
435+
/// legacy `SOCKET_PATCH_TELEMETRY_DISABLED` name), so the airgap tests
436+
/// below can't pass or fail vacuously. Restores afterwards.
437+
fn with_clean_telemetry_env(f: impl FnOnce()) {
438+
const EXTRA: &[&str] = &["VITEST", "SOCKET_PATCH_TELEMETRY_DISABLED"];
439+
let saved: Vec<(&str, Option<String>)> =
440+
EXTRA.iter().map(|&k| (k, std::env::var(k).ok())).collect();
441+
for &k in EXTRA {
442+
std::env::remove_var(k);
443+
}
444+
f();
445+
for (k, v) in saved {
446+
match v {
447+
Some(v) => std::env::set_var(k, v),
448+
None => std::env::remove_var(k),
449+
}
450+
}
451+
}
452+
453+
/// `--offline` promises "never contact the network", but the telemetry
454+
/// kill-switch (`socket_patch_core::utils::telemetry::is_telemetry_disabled`)
455+
/// reads the `SOCKET_OFFLINE` env var directly — it never sees the parsed
456+
/// flag. `apply_env_toggles` must therefore mirror `--offline` into the
457+
/// env exactly like `--debug` / `--no-telemetry`, or an airgapped
458+
/// `socket-patch apply --offline` still fires a telemetry HTTP request.
459+
#[test]
460+
#[serial_test::serial]
461+
fn apply_env_toggles_mirrors_offline_into_env_for_airgap() {
462+
with_clean_socket_env(|| {
463+
with_clean_telemetry_env(|| {
464+
let args = GlobalArgs {
465+
offline: true,
466+
..GlobalArgs::default()
467+
};
468+
apply_env_toggles(&args);
469+
assert_eq!(std::env::var("SOCKET_OFFLINE").as_deref(), Ok("1"));
470+
assert!(
471+
socket_patch_core::utils::telemetry::is_telemetry_disabled(),
472+
"--offline must disable telemetry (strict airgap: never contact the network)",
473+
);
474+
});
475+
});
476+
}
477+
478+
/// The full `SOCKET_OFFLINE` vocabulary must reach the telemetry gate.
479+
/// clap (via `parse_bool_flag`) accepts `yes`/`on`/`y`/`t` as true, but
480+
/// core's direct env read matches only `"1" | "true"` — so the toggle
481+
/// mirror has to re-export the parsed flag in normalized form.
482+
#[test]
483+
#[serial_test::serial]
484+
fn truthy_offline_env_vocabulary_reaches_telemetry_gate() {
485+
with_clean_socket_env(|| {
486+
with_clean_telemetry_env(|| {
487+
std::env::set_var("SOCKET_OFFLINE", "yes");
488+
let cli = TestCli::try_parse_from(["socket-patch"]).unwrap();
489+
assert!(cli.common.offline, "SOCKET_OFFLINE=yes parses as offline");
490+
apply_env_toggles(&cli.common);
491+
assert!(
492+
socket_patch_core::utils::telemetry::is_telemetry_disabled(),
493+
"SOCKET_OFFLINE=yes must disable telemetry like SOCKET_OFFLINE=1",
494+
);
495+
});
496+
});
497+
}
498+
499+
/// `scrub_empty_global_env_vars` removes exactly-empty `SOCKET_*` globals
500+
/// (the `VAR=` blank-without-unsetting idiom) and nothing else: set,
501+
/// non-empty values — even whitespace-only ones, which are significant in
502+
/// paths — survive, and the previously-crashing parse then sees plain
503+
/// defaults.
504+
#[test]
505+
#[serial_test::serial]
506+
fn scrub_empty_global_env_vars_unsets_only_empties() {
507+
with_clean_socket_env(|| {
508+
std::env::set_var("SOCKET_CWD", "");
509+
std::env::set_var("SOCKET_LOCK_TIMEOUT", "");
510+
std::env::set_var("SOCKET_GLOBAL_PREFIX", "");
511+
std::env::set_var("SOCKET_ECOSYSTEMS", "");
512+
std::env::set_var("SOCKET_DOWNLOAD_MODE", "");
513+
std::env::set_var("SOCKET_MANIFEST_PATH", "keep.json");
514+
std::env::set_var("SOCKET_ORG_SLUG", " ");
515+
516+
scrub_empty_global_env_vars();
517+
518+
assert!(
519+
std::env::var("SOCKET_CWD").is_err(),
520+
"empty var is scrubbed"
521+
);
522+
assert!(std::env::var("SOCKET_LOCK_TIMEOUT").is_err());
523+
assert_eq!(
524+
std::env::var("SOCKET_MANIFEST_PATH").as_deref(),
525+
Ok("keep.json"),
526+
"non-empty values must survive the scrub",
527+
);
528+
assert_eq!(
529+
std::env::var("SOCKET_ORG_SLUG").as_deref(),
530+
Ok(" "),
531+
"whitespace-only values are left for the parsers to judge",
532+
);
533+
534+
let cli = TestCli::try_parse_from(["socket-patch"])
535+
.expect("blank env vars must mean 'unset', not a parse abort");
536+
assert_eq!(cli.common.cwd, PathBuf::from("."));
537+
assert_eq!(cli.common.lock_timeout, None);
538+
assert!(cli.common.global_prefix.is_none());
539+
assert!(cli.common.ecosystems.is_none());
540+
assert_eq!(cli.common.download_mode, "diff");
541+
assert_eq!(cli.common.manifest_path, "keep.json");
542+
});
543+
}
544+
395545
/// `parse_bool_flag` accepts the same vocabulary as clap's
396546
/// `BoolishValueParser`, case-insensitively and with surrounding whitespace
397547
/// trimmed.

crates/socket-patch-cli/src/commands/apply.rs

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -772,9 +772,31 @@ async fn apply_patches_inner(
772772
// Resolve patch sources (read `.socket/` directly, or stage an overlay
773773
// tempdir + download the gap). Shared with `vendor` via fetch_stage.
774774
let socket_dir = manifest_path.parent().unwrap();
775+
// Partition manifest PURLs by ecosystem up front. The source probes,
776+
// the offline guard, and the download planner in `fetch_stage` must only
777+
// consider patches this run can actually apply — the `--ecosystems`
778+
// filter plus the ecosystems compiled into this build. An out-of-scope
779+
// patch with no local source must not fail (or trigger fetches for) a
780+
// run that will never apply it.
781+
let manifest_purls: Vec<String> = manifest.patches.keys().cloned().collect();
782+
let partitioned = partition_purls(&manifest_purls, args.common.ecosystems.as_deref());
783+
784+
let target_manifest_purls: HashSet<String> = partitioned
785+
.values()
786+
.flat_map(|purls| purls.iter().cloned())
787+
.collect();
788+
789+
// In-scope view of the manifest for source probing and fetching. The
790+
// apply loop keeps using the full `manifest` for per-PURL lookups —
791+
// those are already scoped by `partitioned`.
792+
let mut scoped_manifest = manifest.clone();
793+
scoped_manifest
794+
.patches
795+
.retain(|purl, _| target_manifest_purls.contains(purl));
796+
775797
let staged = match crate::commands::fetch_stage::stage_patch_sources(
776798
&args.common,
777-
&manifest,
799+
&scoped_manifest,
778800
socket_dir,
779801
)
780802
.await?
@@ -788,15 +810,6 @@ async fn apply_patches_inner(
788810
let diffs_path = staged.diffs.clone();
789811
let packages_path = staged.packages.clone();
790812

791-
// Partition manifest PURLs by ecosystem
792-
let manifest_purls: Vec<String> = manifest.patches.keys().cloned().collect();
793-
let partitioned = partition_purls(&manifest_purls, args.common.ecosystems.as_deref());
794-
795-
let target_manifest_purls: HashSet<String> = partitioned
796-
.values()
797-
.flat_map(|purls| purls.iter().cloned())
798-
.collect();
799-
800813
// Vendor ownership wins for EVERY ecosystem: a purl recorded in
801814
// `.socket/vendor/state.json` is managed by the explicit `vendor`
802815
// action — apply must not re-patch its installed tree (or repoint a

0 commit comments

Comments
 (0)