Skip to content

Commit 460b1bf

Browse files
mikolalysenkoclaude
andcommitted
fix(gem): surface the config-root skip on the run warning channels; class-split fallback-home failure semantics
Review-round fixes for the two verified Bugbot findings on #222. Finding 1 (Medium): the containment guard's `gem_bundle_config_path_ignored` was a bare eprintln inside the crawler — it never reached any --json `warnings[]` and printed under --silent, violating the repo-wide warning conventions (#219/#220 + the #223 omnibus silent fixes). The crawler is now print-free: the refusal is RECORDED on `BundleStoreDiscovery.skipped_config_path`, and a shared `config_path_ignored_warning(value)` builder feeds the CLI channels — scan pushes it onto the same run-level channel as the PnP layout refusals (JSON `warnings[]` on both the zero-package and >=1-package envelopes; one stderr line gated on !json && !silent), and apply carries it in its Envelope `warnings[]` plus one gated stderr line. Scoped like the crawl that hit it: local mode, gem in --ecosystems/manifest scope. Finding 2 (High, with nuance): with an env/config bundle root, get_gem_paths appends the gem-env fallback homes and the multi-copy fan-out patched EVERY copy with per-copy loud-fail — so a gem present in both the bundle store and a shared home (rvm @global, root-owned system dir) failed the WHOLE run on a permission failure or variant mismatch THERE, even though the copy bundler loads patched fine. Patching a shared home's copy is not itself wrong (plain apply always patched GEM_HOME when no store existed); the defect was failure semantics crossing store classes. Fix: discovery's store list is exposed (`RubyCrawler::discover_bundle_stores`, fs-probes only) and apply's gem fan-out classes each copy — bundle-path store copies stay PRIMARY (loud-fail, unchanged); gem-env fallback-home copies become BEST-EFFORT once at least one store copy applied: a variant mismatch or write failure there is a per-copy non-fatal Skipped event (`gem_fallback_home_skipped`, detail names the path and reason; gated stderr twin), never a run failure. Parity edge kept: with NO bundle-store copy (the historic fallback-only layout, and every --global run) the home copy IS primary and keeps loud-fail exactly as pre-#218 apply. TDD evidence (red -> green on the rebased tip): scan/apply --json missing the warnings[] entry and the --silent leak (3 tests, in_process_gem_config_warning.rs); mismatched-home-copy exit 1 and Failed-event-on-strict-refusal (in_process_gem_fallback_home.rs), with fallback-only loud parity and both-copies-patched pinned green throughout. CLI_CONTRACT.md documents the copy classes and the warning channels. Gates: touched files rustfmt-clean; cargo clippy --workspace --all-features -D warnings clean; core --lib 2535, cli --lib 436; crawlers::ruby 50, crawler_ruby_e2e 25; gem+npm multicopy 2+2; apply_network 11, apply_invariants 4, cli_gem_variant_mismatch_policy 6, cli_apply_silent 2, e2e_gem hermetic 8 (6 shown +cache selftests), e2e_scan, cli_scan_silent, docker_e2e_gem — all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7e20227 commit 460b1bf

7 files changed

Lines changed: 943 additions & 60 deletions

File tree

crates/socket-patch-cli/CLI_CONTRACT.md

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -292,15 +292,29 @@ the model is **not uniform** today:
292292
`<root>/<engine>/<abi>/gems/` and flat `<root>/gems/`). The env variable is the user's own machine
293293
state, so it is honored verbatim (it may point outside `--cwd`; a leading `~` expands against home);
294294
the **config file is typically committed — untrusted input — so a config-sourced root that resolves
295-
outside the project root is skipped with a `gem_bundle_config_path_ignored` stderr warning**
296-
(`BUNDLE_PATH__SYSTEM: "true"` likewise drops the recorded path, as bundler itself ignores it).
297-
Explicit env/config roots only count when `--cwd` holds a Bundler manifest/lockfile. When the
298-
default `vendor/bundle` root holds no store, the gem homes `gem env` reports are appended (default
299-
gems like rexml/json only ever live there). When several roots hold **coexisting physical copies of
300-
one `gem@version`** (bundler-2's scoped store beside bundler-1's flat store), `apply`/`rollback`
301-
patch/restore **every copy** — one summary event per copy, mirroring npm's multi-copy fan-out —
302-
while single-representative consumers (`get`, `vendor`, `setup`, `vex`) use the
303-
highest-precedence copy.
295+
outside the project root is skipped** (`BUNDLE_PATH__SYSTEM: "true"` likewise drops the recorded
296+
path, as bundler itself ignores it). The skip is surfaced per the run-warning conventions: a
297+
`gem_bundle_config_path_ignored` entry in the run-level `warnings[]` of `scan`/`apply` `--json`
298+
envelopes (detail names the config value and the env-`BUNDLE_PATH` remedy), and one stderr
299+
`Warning (gem_bundle_config_path_ignored): …` line on the human path, gated on `!--silent`
300+
(`--silent` = errors only). Explicit env/config roots only count when `--cwd` holds a Bundler
301+
manifest/lockfile. When the default `vendor/bundle` root holds no store, the gem homes `gem env`
302+
reports are appended (default gems like rexml/json only ever live there). When several roots hold
303+
**coexisting physical copies of one `gem@version`** (bundler-2's scoped store beside bundler-1's
304+
flat store), `apply`/`rollback` patch/restore **every copy** — one summary event per copy,
305+
mirroring npm's multi-copy fan-out — while single-representative consumers (`get`, `vendor`,
306+
`setup`, `vex`) use the highest-precedence copy.
307+
308+
*Copy classes (additive to the multi-copy vocabulary):* a copy under a **bundle-path store**
309+
(config/env/default root) is PRIMARY — a variant mismatch or write failure there fails the run,
310+
as always. A copy in a **`gem env` fallback home** (rvm `@global`, `--user-install`, system gem
311+
dirs — shared, often root-owned) is patched too when it matches and is writable, but becomes
312+
BEST-EFFORT once at least one bundle-store copy applied: its mismatch/write failure surfaces as a
313+
non-fatal `skipped` event (`errorCode: gem_fallback_home_skipped`, detail names the copy's path
314+
and reason; gated stderr twin on the human path) instead of failing a run whose loaded copy is
315+
patched. With **no** bundle-store copy (the historic fallback-only layout, and every `--global`
316+
run) the fallback-home copy IS the primary install and keeps loud-fail parity with pre-bundle-path
317+
`apply`.
304318

305319
**Intended (gap):** the cwd-only ecosystems *should* also auto-discover per-subproject lockfiles when
306320
run from the repo root, matching the npm workspace model. The npm-vs-others asymmetry is a known

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

Lines changed: 220 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
use clap::Args;
22
use socket_patch_core::api::client::get_api_client_with_overrides;
3+
use socket_patch_core::crawlers::ruby_crawler::config_path_ignored_warning;
34
use socket_patch_core::crawlers::{
4-
detect_npm_pkg_manager, CrawlerOptions, Ecosystem, NpmPkgManager,
5+
detect_npm_pkg_manager, CrawlerOptions, Ecosystem, NpmPkgManager, RubyCrawler,
56
};
67
use socket_patch_core::manifest::operations::read_manifest;
78
use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord};
@@ -24,8 +25,8 @@ use crate::commands::lock_cli::acquire_or_emit;
2425
use crate::commands::vex::{generate_vex_from_manifest_path, VexEmbedArgs};
2526
use crate::ecosystem_dispatch::{find_all_packages_for_purls, partition_purls};
2627
use crate::json_envelope::{
27-
AppliedVia, Command, Envelope, EnvelopeError, PatchAction, PatchEvent, PatchEventFile, Status,
28-
VexSummary,
28+
AppliedVia, Command, Envelope, EnvelopeError, PatchAction, PatchEvent, PatchEventFile,
29+
RunWarning, Status, VexSummary,
2930
};
3031

3132
/// Files whose pre-apply content matched NEITHER hash and were (or would
@@ -770,12 +771,31 @@ pub async fn run(args: ApplyArgs) -> i32 {
770771
}
771772

772773
match apply_patches_inner(&args, &manifest_path).await {
773-
Ok((success, results, unmatched)) => {
774+
Ok(ApplyOutcome {
775+
success,
776+
results,
777+
unmatched,
778+
run_warnings,
779+
fallback_skips,
780+
}) => {
774781
let patched_count = results
775782
.iter()
776783
.filter(|r| r.success && !r.files_patched.is_empty())
777784
.count();
778785

786+
// Run-level advisories + best-effort fallback-home skips on the
787+
// human path: one gated stderr line each. `--silent` is
788+
// errors-only, and under `--json` the envelope copies below are
789+
// the machine channel — same gating as scan's run warnings.
790+
if !args.common.json && !args.common.silent {
791+
for w in &run_warnings {
792+
eprintln!("Warning ({}): {}", w.code, w.detail);
793+
}
794+
for skip in &fallback_skips {
795+
eprintln!("Warning (gem_fallback_home_skipped): {}", skip.detail());
796+
}
797+
}
798+
779799
// Embedded VEX: only on a successful apply and only when
780800
// `--vex <path>` was passed. Re-read the manifest fresh so
781801
// verification observes the just-applied on-disk state. The
@@ -835,6 +855,20 @@ pub async fn run(args: ApplyArgs) -> i32 {
835855
),
836856
);
837857
}
858+
// Best-effort gem-env fallback-home copies left unpatched:
859+
// one non-fatal Skipped event each, with the copy's path
860+
// and reason. Never a Failed event — the bundle-store copy
861+
// (the one bundler loads) applied, so the run stands.
862+
for skip in &fallback_skips {
863+
env.record(
864+
PatchEvent::new(PatchAction::Skipped, skip.purl.clone())
865+
.with_reason("gem_fallback_home_skipped", skip.detail()),
866+
);
867+
}
868+
// Run-level advisories (the gem config-root containment
869+
// skip): the envelope's `warnings[]` is their machine
870+
// channel — stderr is suppressed under --json.
871+
env.warnings.extend(run_warnings.iter().cloned());
838872
if !success {
839873
env.mark_partial_failure();
840874
}
@@ -1086,10 +1120,54 @@ fn unmatched_purls(
10861120
.collect()
10871121
}
10881122

1123+
/// Everything `apply_patches_inner` reports back to `run`'s output
1124+
/// builders (JSON envelope + human summary).
1125+
struct ApplyOutcome {
1126+
/// Overall success — `false` fails the command (exit 1 /
1127+
/// `partialFailure`).
1128+
success: bool,
1129+
results: Vec<ApplyResult>,
1130+
/// In-scope manifest purls with no installed package on disk.
1131+
unmatched: Vec<String>,
1132+
/// Run-level advisories: JSON `warnings[]`, one gated stderr line each
1133+
/// on the human path (`--silent` = errors only). Today: the gem
1134+
/// config-root containment skip.
1135+
run_warnings: Vec<RunWarning>,
1136+
/// Gem-env fallback-home copies deliberately left unpatched
1137+
/// (best-effort class): one non-fatal `Skipped` event each in the
1138+
/// envelope, one gated stderr line each on the human path.
1139+
fallback_skips: Vec<FallbackHomeSkip>,
1140+
}
1141+
1142+
/// One gem-env fallback-home copy the fan-out skipped best-effort (a
1143+
/// bundle-store copy applied; this shared-home copy mismatched or failed
1144+
/// to write). Carries what the warning must name: the package, the copy's
1145+
/// path, and why.
1146+
struct FallbackHomeSkip {
1147+
purl: String,
1148+
path: PathBuf,
1149+
why: String,
1150+
}
1151+
1152+
impl FallbackHomeSkip {
1153+
/// The human/detail text shared by the JSON event reason and the
1154+
/// stderr line.
1155+
fn detail(&self) -> String {
1156+
format!(
1157+
"gem-env home copy at {} was not patched ({}); the project's \
1158+
bundle-path copy — the one bundler loads — is patched. Shared \
1159+
gem homes are machine-wide state: patch them explicitly with \
1160+
`--global` if desired",
1161+
self.path.display(),
1162+
self.why
1163+
)
1164+
}
1165+
}
1166+
10891167
async fn apply_patches_inner(
10901168
args: &ApplyArgs,
10911169
manifest_path: &Path,
1092-
) -> Result<(bool, Vec<ApplyResult>, Vec<String>), String> {
1170+
) -> Result<ApplyOutcome, String> {
10931171
let manifest = read_manifest(manifest_path)
10941172
.await
10951173
.map_err(|e| e.to_string())?
@@ -1124,7 +1202,15 @@ async fn apply_patches_inner(
11241202

11251203
let mut staged = match stage_patch_sources(&args.common, &scoped_manifest, socket_dir).await? {
11261204
StageOutcome::Ready(s) => s,
1127-
StageOutcome::Unavailable => return Ok((false, Vec::new(), Vec::new())),
1205+
StageOutcome::Unavailable => {
1206+
return Ok(ApplyOutcome {
1207+
success: false,
1208+
results: Vec::new(),
1209+
unmatched: Vec::new(),
1210+
run_warnings: Vec::new(),
1211+
fallback_skips: Vec::new(),
1212+
})
1213+
}
11281214
};
11291215

11301216
// Vendor ownership wins for EVERY ecosystem: a purl recorded in
@@ -1153,6 +1239,45 @@ async fn apply_patches_inner(
11531239
global_prefix: args.common.global_prefix.clone(),
11541240
};
11551241

1242+
// Gem bundle-store discovery, re-run cheaply (filesystem probes only,
1243+
// no `gem env` shell-out) against the same ambient environment the
1244+
// crawler reads, for two consumers:
1245+
// * the config-skip advisory — a committed `.bundle/config` whose
1246+
// BUNDLE_PATH the containment guard refused must surface on the
1247+
// run's warning channels (JSON `warnings[]`; gated stderr), not
1248+
// vanish silently;
1249+
// * the store-class boundary for the gem fan-out below — copies
1250+
// under a bundle-path store are primary, everything else is a
1251+
// `gem env` fallback-home copy (best-effort once a store copy
1252+
// applied).
1253+
// Only when this run actually crawls gems locally: a --global run or
1254+
// one whose `--ecosystems`/manifest scope holds no gem purls never
1255+
// consults the config, so it must not warn about it either.
1256+
let gem_discovery = if partitioned.contains_key(&Ecosystem::Gem)
1257+
&& !args.common.global
1258+
&& args.common.global_prefix.is_none()
1259+
{
1260+
Some(RubyCrawler::discover_bundle_stores(&args.common.cwd).await)
1261+
} else {
1262+
None
1263+
};
1264+
let gem_store_dirs: &[PathBuf] = gem_discovery
1265+
.as_ref()
1266+
.map(|d| d.stores.as_slice())
1267+
.unwrap_or(&[]);
1268+
let mut run_warnings: Vec<RunWarning> = Vec::new();
1269+
if let Some(value) = gem_discovery
1270+
.as_ref()
1271+
.and_then(|d| d.skipped_config_path.as_deref())
1272+
{
1273+
let (code, detail) = config_path_ignored_warning(value);
1274+
run_warnings.push(RunWarning {
1275+
code: code.to_string(),
1276+
detail,
1277+
});
1278+
}
1279+
let mut fallback_skips: Vec<FallbackHomeSkip> = Vec::new();
1280+
11561281
// Multi-copy aware: npm nests genuine duplicates of one `name@version`
11571282
// (nested dupes, diamonds, `file:` dups), so the resolver returns EVERY
11581283
// physical copy per PURL. Patching only one would leave a live,
@@ -1175,7 +1300,13 @@ async fn apply_patches_inner(
11751300
if !args.common.silent && !args.common.json {
11761301
println!("No patches to apply.");
11771302
}
1178-
return Ok((true, Vec::new(), Vec::new()));
1303+
return Ok(ApplyOutcome {
1304+
success: true,
1305+
results: Vec::new(),
1306+
unmatched: Vec::new(),
1307+
run_warnings,
1308+
fallback_skips,
1309+
});
11791310
}
11801311

11811312
if all_packages.is_empty() {
@@ -1198,7 +1329,13 @@ async fn apply_patches_inner(
11981329
" Check that packages are installed and --cwd points to the right directory."
11991330
);
12001331
}
1201-
return Ok((unmatched.is_empty(), results, unmatched));
1332+
return Ok(ApplyOutcome {
1333+
success: unmatched.is_empty(),
1334+
results,
1335+
unmatched,
1336+
run_warnings,
1337+
fallback_skips,
1338+
});
12021339
}
12031340

12041341
// Apply patches
@@ -1274,8 +1411,30 @@ async fn apply_patches_inner(
12741411
std::slice::from_ref(pkg_path)
12751412
};
12761413

1414+
// Copy CLASS decides FAILURE semantics (never write scope —
1415+
// patching a shared home's vulnerable copy is fine when it
1416+
// works): bundle-path store copies are PRIMARY and loud-fail;
1417+
// `gem env` fallback-home copies (rvm `@global`, system gem
1418+
// dirs — often root-owned, shared machine-wide) are
1419+
// BEST-EFFORT once a store copy applied — a mismatch or write
1420+
// failure there becomes a non-fatal per-copy Skipped warning,
1421+
// because the copy bundler actually loads is already patched.
1422+
// With NO store copy the fallback home IS the primary install
1423+
// (the historic pre-bundle-path layout, and every --global
1424+
// run) and keeps loud-fail parity. Store copies run first so
1425+
// best-effort is decidable when the fallback copies come up.
1426+
let (store_copies, home_copies): (Vec<&PathBuf>, Vec<&PathBuf>) = copy_paths
1427+
.iter()
1428+
.partition(|p| gem_store_dirs.iter().any(|s| p.starts_with(s)));
1429+
let mut any_store_copy_applied = false;
1430+
12771431
let mut any_copy_applied = false;
1278-
for pkg_path in copy_paths {
1432+
for (pkg_path, is_store_copy) in store_copies
1433+
.into_iter()
1434+
.map(|p| (p, true))
1435+
.chain(home_copies.into_iter().map(|p| (p, false)))
1436+
{
1437+
let best_effort = !is_store_copy && any_store_copy_applied;
12791438
let mut applied = false;
12801439
// Did at least one variant reach `apply_package_patch`? A
12811440
// variant reaches it only after passing the first-file
@@ -1284,9 +1443,9 @@ async fn apply_patches_inner(
12841443
// not be reported as "package_not_installed" even if the patch
12851444
// itself then fails. Tracks the "matched but failed" case so the
12861445
// failure message is honest and `unmatched` stays accurate.
1287-
// Both are PER COPY: a copy that matches no variant must fail
1288-
// loudly even when a sibling copy applied cleanly — that copy
1289-
// is a real on-disk gem some bundler loads.
1446+
// Both are PER COPY: a primary copy that matches no variant
1447+
// must fail loudly even when a sibling copy applied cleanly —
1448+
// that copy is a real on-disk gem some bundler loads.
12901449
let mut attempted = false;
12911450

12921451
for variant_purl in &variants {
@@ -1358,11 +1517,28 @@ async fn apply_patches_inner(
13581517
matched_manifest_purls.insert(variant_purl.clone());
13591518
if result.success {
13601519
applied = true;
1520+
results.push(result);
13611521
// No `break`: apply *every* matching variant. PyPI/gem
13621522
// have exactly one installed distribution (the rest
13631523
// hash-mismatch and were skipped above), so this
13641524
// applies a single variant for them; Maven's coexisting
13651525
// classifier jars each get patched.
1526+
} else if best_effort {
1527+
// A write failure on a BEST-EFFORT fallback-home copy
1528+
// (root-owned rvm `@global`, a system gem dir) is a
1529+
// per-copy non-fatal skip, never a run failure: the
1530+
// bundle-store copy — the one bundler loads — already
1531+
// applied. The failed result is NOT recorded (its
1532+
// Failed event would flip `partialFailure`); the skip
1533+
// rides `fallback_skips` into the envelope instead.
1534+
fallback_skips.push(FallbackHomeSkip {
1535+
purl: base_purl.clone(),
1536+
path: pkg_path.clone(),
1537+
why: result
1538+
.error
1539+
.clone()
1540+
.unwrap_or_else(|| "unknown error".to_string()),
1541+
});
13661542
} else {
13671543
// A variant that reached apply IS the installed
13681544
// distribution, so a failure here is a real apply
@@ -1382,19 +1558,37 @@ async fn apply_patches_inner(
13821558
result.error.as_deref().unwrap_or("unknown error")
13831559
);
13841560
}
1561+
results.push(result);
13851562
}
1386-
results.push(result);
13871563
}
13881564

13891565
if applied {
13901566
any_copy_applied = true;
1567+
if is_store_copy {
1568+
any_store_copy_applied = true;
1569+
}
1570+
} else if best_effort {
1571+
// Nothing applied on a best-effort fallback-home copy.
1572+
// Attempted-but-failed variants already recorded their
1573+
// per-copy skip above; a copy no variant matched gets
1574+
// one here — the shared home holds a different (or
1575+
// locally diverged) distribution, and the copy bundler
1576+
// loads is patched, so this is advisory, not an error.
1577+
if !attempted {
1578+
fallback_skips.push(FallbackHomeSkip {
1579+
purl: base_purl.clone(),
1580+
path: pkg_path.clone(),
1581+
why: "no release variant in the manifest matches this copy".to_string(),
1582+
});
1583+
}
13911584
} else {
1392-
// Nothing applied for this copy. `has_errors` was already set
1393-
// per-variant above when a variant was attempted-but-failed;
1394-
// set it here too for the no-variant-attempted case so both
1395-
// paths fail the command — per copy, so a second store copy
1396-
// that matches no variant fails loudly instead of silently
1397-
// staying vulnerable behind a sibling copy's success.
1585+
// Nothing applied for this PRIMARY copy. `has_errors` was
1586+
// already set per-variant above when a variant was
1587+
// attempted-but-failed; set it here too for the
1588+
// no-variant-attempted case so both paths fail the command
1589+
// — per copy, so a second store copy that matches no
1590+
// variant fails loudly instead of silently staying
1591+
// vulnerable behind a sibling copy's success.
13981592
has_errors = true;
13991593
if !attempted && !args.common.silent && !args.common.json {
14001594
// No variant matched the installed distribution at all —
@@ -1523,7 +1717,13 @@ async fn apply_patches_inner(
15231717
// means it can run repeatedly (CI dry-runs, deploy hooks) without
15241718
// mutating patch state.
15251719

1526-
Ok((!has_errors, results, unmatched))
1720+
Ok(ApplyOutcome {
1721+
success: !has_errors,
1722+
results,
1723+
unmatched,
1724+
run_warnings,
1725+
fallback_skips,
1726+
})
15271727
}
15281728

15291729
#[cfg(test)]

0 commit comments

Comments
 (0)