feat(apply): safety hardening — atomicity, locking, pnpm CoW, sidecars, Maven gate#80
Open
Mikola Lysenko (mikolalysenko) wants to merge 57 commits into
Open
feat(apply): safety hardening — atomicity, locking, pnpm CoW, sidecars, Maven gate#80Mikola Lysenko (mikolalysenko) wants to merge 57 commits into
Mikola Lysenko (mikolalysenko) wants to merge 57 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Mikola Lysenko (mikolalysenko)
force-pushed
the
feat/apply-safety-hardening
branch
from
b148a97 to
15639fe
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
Author
|
Update: pushed
Notable wires:
Local validation (all green): Shared helpers live in |
Adds five new modules to `socket-patch-core` and refactors `apply_file_patch` to compose them safely with #79's perm-preservation: - **`patch::apply_lock`** — cross-platform advisory file lock at `<.socket>/apply.lock` via `fs2`. Used by every mutating subcommand to serialize against concurrent socket-patch runs. - **`patch::cow`** — hardlink + symlink copy-on-write. Before patching, if `filepath` is a symlink into a content-addressed store (pnpm) or a regular file with `nlink > 1` (bazel mirrors, nix store overlays), give this project a private inode. The pnpm content store and every other project pointing at it stay byte-identical. - **`patch::sidecars`** — ecosystem-aware sidecar fixups dispatched from `apply_package_patch`. Cargo: rewrite `.cargo-checksum.json` with new SHA256s so `cargo build` accepts patched sources. NuGet: delete `.nupkg.metadata` (the documented "unknown" state vs. a stale `contentHash` that would flag tampering). PyPI / gem / Go: advisory-only — surface a one-line note about downstream tooling consequences. - **`crawlers::pkg_managers`** — path-based detector for the four Node.js layout flavors (npm / pnpm / yarn-classic / yarn-berry PnP). Apply uses this to refuse yarn-berry PnP (packages live in `.yarn/cache/*.zip`) and to surface a pnpm-detected note. - **`apply_file_patch` atomic rewrite** — two-phase commit: 1. Hash `patched_content` in memory; error out before any disk write if it doesn't match `expected_hash`. Removes the prior "wrote bytes, post-write verify failed, can't restore" window. 2. CoW the target if it's a shared inode. 3. Stage write to `<parent>/.socket-stage-<uuid>`, `sync_all()`, then `rename(stage, target)`. POSIX `rename(2)` is atomic — observers see either the old or new bytes, never a truncated half-write. Composes cleanly with #79's mode + uid/gid restore step which now operates on the post-rename inode. `ApplyResult` grows `sidecars_updated: Vec<String>` and `sidecar_advisory: Option<String>` so the CLI envelope can surface fixup outcomes. `fs2` and `tempfile` added to socket-patch-core dependencies. Two new tests pin the headline invariants: - `test_apply_file_patch_hash_mismatch_leaves_original_intact` — atomic-write contract: hash mismatch leaves target byte-identical AND no `.socket-stage-*` litter in parent dir. - `test_apply_file_patch_does_not_propagate_to_hardlinked_sibling` — the pnpm content-store invariant at the integration level. Plus 10 unit tests for cow + apply_lock and 13 for sidecars/* + 9 for pkg_managers. Assisted-by: Claude Code:claude-opus-4-7
Integrates the new socket-patch-core safety primitives into the CLI via the v3.0 unified `GlobalArgs` + `Envelope` patterns from #79. **`commands::lock_cli`** (new) — envelope-aware wrapper around `apply_lock::acquire`. Takes `Command` so the failure envelope's `command` field reflects which subcommand was blocked. On contention the binary emits `{status: "error", error: {code: "lock_held", ...}}` in JSON mode or a one-line stderr message otherwise, then exits 1. **Lock acquisition** added to `apply`, `rollback`, `repair`, `remove` immediately after the manifest existence check. `remove`'s outer lock spans the inner `rollback_patches` call (which deliberately does NOT acquire the lock so the composition doesn't self-deadlock). **Apply pkg-manager gating** — after the lock, `apply` runs `detect_npm_pkg_manager`: - `YarnBerryPnP` → emit `EnvelopeError("yarn_pnp_unsupported", ...)` pointing at `yarn patch` and exit 1. - `Pnpm` → surface a one-line stderr note. CoW handles the substantive safety work; this just tells the user the layout was understood. **Sidecar JSON via `event.details`** — `result_to_event` extends the Applied event with `details.sidecarsUpdated: string[]` and `details.sidecarAdvisory: string | null` when either is non-empty. Narrower JSON-envelope contract than first-class fields; consumers read `event.details.sidecarsUpdated` from JSON. **Maven + NuGet experimental runtime gates** in `ecosystem_dispatch.rs`. Even when compiled with `--features maven`/`nuget`, the crawlers refuse to dispatch unless the matching `SOCKET_EXPERIMENTAL_MAVEN=1`/`SOCKET_EXPERIMENTAL_NUGET=1` env var is set. Without it, surface a warning event and skip those PURLs. Reasoning: Maven patches corrupt jar sidecar checksums (sha1/md5); NuGet patches corrupt `.nupkg.sha512` signature sidecars that `dotnet restore` reads as tamper-evidence. `fs2` added to socket-patch-cli dev-dependencies for the lock e2e test (same crate the binary uses internally). Assisted-by: Claude Code:claude-opus-4-7
Adds four end-to-end integration test files exercising the safety
primitives through the binary, plus shared `tests/common/mod.rs`
helpers, plus two existing-test contract updates.
**Suites added (20 new tests):**
- `e2e_safety_lock.rs` (6 tests, non-ignored). Test holds the same
`.socket/apply.lock` the binary uses via `fs2` directly, then
spawns `socket-patch apply` and asserts the second process exits
with `error.code == "lock_held"`. Zero production-code hooks.
- `e2e_safety_yarn_pnp.rs` (5 tests, non-ignored). Yarn-berry PnP
markers (`.pnp.cjs`, `.pnp.loader.mjs`) trigger
`error.code == "yarn_pnp_unsupported"`. Negative control:
plain npm layout does NOT trigger the refusal.
- `e2e_safety_cargo_build.rs` (5 tests, `#[ignore]` + `--features
cargo`). Three synthetic-vendor tests:
1. Baseline `cargo check --offline --frozen` succeeds.
2. Negative control — mutating the source WITHOUT the sidecar
fixup makes cargo refuse with "checksum changed". Proves
cargo actually verifies, which is what makes the positive
test meaningful.
3. Sidecar fixup makes `cargo check` pass; `.cargo-checksum.json`
is rewritten and the `package` field is preserved.
4. JSON envelope contract: `.cargo-checksum.json` appears in
`event.details.sidecarsUpdated`.
Plus `traitobject_real_socket_patch_round_trip` — the cargo
layer-2+3 combined test: `cargo fetch traitobject@0.0.1` from
crates.io → `socket-patch get b15f2b7f-d5cb-43c9-b793-80f71682188f`
from patches-api.socket.dev → assert `.cargo-checksum.json`
rewritten + `cargo check` succeeds against the real, production
Socket patch.
- `e2e_safety_pnpm.rs` (4 tests, `#[ignore]`). Two projects share a
pnpm content store via `--config.package-import-method=hardlink`.
`socket-patch get` in project A patches A; project B + store
entry stay byte-identical. `pnpm install --frozen-lockfile` in B
afterwards does not revert A. Exercises CoW against a real pnpm
install rather than a hand-rolled hardlink.
**`tests/common/mod.rs`** — shared helpers (`binary`, `run`,
`assert_run_ok`, `git_sha256`, `sha256_hex`, `pnpm_run`, `cargo_run`,
`write_minimal_manifest`, `write_blob`, `parse_json_envelope`,
`envelope_error_code`, `envelope_error_message`) lifted from the
duplicated copies in `e2e_npm.rs` etc. Additive; existing suites
keep their inlined copies for now.
**CI matrix** in `.github/workflows/ci.yml`:
- `e2e_safety_cargo_build` on ubuntu + macos + windows
- `e2e_safety_pnpm` on ubuntu + macos + windows
(pnpm-on-Windows uses junctions + copies by default, so the CoW
invariant holds vacuously; the test still runs to verify apply
doesn't error on Windows. Semantic Windows nlink coverage is a
follow-up — `std::fs::Metadata` doesn't expose nlink on Windows
without `GetFileInformationByHandle` via `windows-sys`.)
- New `Setup pnpm` step (`npm install -g pnpm@10`) gated on the
pnpm suite. The fast non-ignored suites (`e2e_safety_lock`,
`e2e_safety_yarn_pnp`) run via the standard `test` job on all
three platforms.
**Existing-test contract updates** (these tests were pinning the
old, broken behavior; both still describe correct invariants —
their assertions just needed to track the rebased semantics):
- `tests/apply_invariants.rs`: `dir_hash` excludes `apply.lock`.
The lock file is deliberate ephemeral session state, not patch
content; the "apply is read-only against .socket/" invariant is
about manifest + blobs + diffs + packages.
- `tests/in_process_edge_cases.rs`:
`apply_blob_after_hash_mismatch_reports_failure` now asserts the
atomic-write contract — the target file is byte-identical to its
pre-call state on the hash-mismatch failure path, no half-written
corruption.
Assisted-by: Claude Code:claude-opus-4-7
Mikola Lysenko (mikolalysenko)
force-pushed
the
feat/apply-safety-hardening
branch
from
e9187cc to
13cbfa7
Compare
Contributor
Author
|
Force-pushed — rebased onto upstream
Notable changes from the previous version of this PR:
Local validation (all green): Depscan PR SocketDev/depscan#20517 is now bumped to |
…+ advisory data
Replaces the previous `event.details.sidecarsUpdated` / `event.details.sidecarAdvisory`
free-form JSON bag with a typed, top-level `Envelope.sidecars[]` list.
## New types (`socket-patch-core/src/patch/sidecars/types.rs`)
pub struct SidecarRecord { purl, ecosystem, files, advisory }
pub struct SidecarFile { path, action: SidecarFileAction }
pub enum SidecarFileAction { Rewritten | Deleted | Created }
pub struct SidecarAdvisory { code, severity, message }
pub enum SidecarAdvisoryCode {
PypiRecordStale | GemBundleInstallReverts | GoModVerifyFails
| NugetSignedPackageTampered | SidecarFixupFailed
}
pub enum SidecarSeverity { Info | Warning | Error }
All derive `serde::Serialize`. Structs use camelCase; enums use
snake_case. Unit tests pin the JSON contract.
## JSON shape (consumer view)
```json
{
"command": "apply",
"events": [...],
"sidecars": [
{ "purl": "pkg:cargo/...", "ecosystem": "cargo",
"files": [{"path":".cargo-checksum.json","action":"rewritten"}] },
{ "purl": "pkg:nuget/...", "ecosystem": "nuget",
"files": [{"path":".nupkg.metadata","action":"deleted"}],
"advisory": { "code":"nuget_signed_package_tampered",
"severity":"warning", "message":"..." } }
]
}
```
- `sidecars` omitted from JSON when empty.
- `files` always present (possibly `[]` for advisory-only).
- `advisory` omitted when absent.
- `code` / `severity` are stable snake_case enum tags; `message`
is human text.
- `purl` joins to `events[].purl` for per-event context.
## Three real improvements over the old design
1. **No more lossy collapse.** NuGet's "deleted `.nupkg.metadata`
AND has a `.nupkg.sha512` signature" case now carries BOTH
a file entry AND an advisory. Before, the advisory was
silently lost when the file entry took its slot.
2. **Stable codes + severity.** Consumers (CI bots, dashboards,
telemetry, jq pipelines) can switch on `code` and route on
`severity` without regex-matching free-form strings.
3. **Decoupled from events.** Sidecar reporting is a top-level
`Envelope.sidecars` list. `PatchEvent.details` is no longer
mixed with `list` / `repair` / `remove`'s command-specific
bags — sidecar consumers have a typed schema all their own.
## Internal refactor
- `SidecarOutcome` removed. Per-ecosystem fixups return
`Result<Option<SidecarPayload>, SidecarError>` (internal
`SidecarPayload = { files, advisory }`); the dispatcher in
`sidecars/mod.rs` wraps the payload with PURL + ecosystem to
produce the `SidecarRecord`.
- `ApplyResult.sidecars_updated: Vec<String>` and
`sidecar_advisory: Option<String>` consolidated into a single
`sidecar: Option<SidecarRecord>` field.
- Apply CLI's `result_to_event` no longer attaches to
`event.details`; the run loop now calls
`env.record_sidecar(record.clone())` after each apply result.
- `Envelope` gains `sidecars: Vec<SidecarRecord>` field +
`record_sidecar` method.
- The error path (`SidecarError` returned by a fixup) is
converted at the apply boundary into a `SidecarRecord` with
`advisory.code = SidecarFixupFailed`, `severity = Error`.
Single uniform shape for consumers.
## Pre-existing test fixups
`in_process_remote_ecosystems_apply.rs` and `in_process_rollback_all_ecosystems.rs`
now set `SOCKET_EXPERIMENTAL_MAVEN=1` / `SOCKET_EXPERIMENTAL_NUGET=1`
when they explicitly exercise those paths. These were broken
silently by the Maven/NuGet runtime gates added in the prior
rebase (the gate was always there in commit 39a2321; tests just
happened not to exercise the maven/nuget paths to a depth where
the skip mattered).
## Test results
- cargo build --workspace --all-features: clean
- cargo build --release --workspace: clean (no warnings)
- cargo clippy --workspace --all-features -- -D warnings: clean
- cargo test --workspace --all-features: 1021 passed, 0 failed
- cargo test --features cargo --test e2e_safety_cargo_build --
--ignored: 5 passed (includes traitobject real-patch round trip)
The e2e cargo test `apply_reports_cargo_checksum_in_sidecars_updated`
tightened from a substring match to a structured-shape assertion
on `envelope.sidecars[].ecosystem=="cargo"` +
`files[].path=".cargo-checksum.json"` + `files[].action=="rewritten"`.
Assisted-by: Claude Code:claude-opus-4-7
Contributor
Author
|
Pushed What changed
Stable enum codes
Severity bucket: Sample consumer queries# All warning-level advisories across the run:
socket-patch apply --json | jq '.sidecars[] | select(.advisory.severity == "warning")'
# Packages that touched .cargo-checksum.json:
socket-patch apply --json | jq -r '.sidecars[] | select(.ecosystem == "cargo") | .purl'
# Count by advisory code (telemetry):
socket-patch apply --json | jq '[.sidecars[].advisory.code // "none"] | group_by(.) | map({code: .[0], count: length})'ValidationThe cargo e2e test Two pre-existing tests in Depscan PR SocketDev/depscan#20517 has been bumped to |
Five test surfaces, one bug fix, one YAGNI cleanup, one harness
simplification — all motivated by closing the e2e gap on the new
typed `Envelope.sidecars[]` contract.
- **e2e_safety_advisories.rs** (new, 5 tests): drive the apply
CLI against handcrafted layouts and assert
`envelope.sidecars[].{ecosystem,advisory.code,advisory.severity,
files[]}` for pypi (`pypi_record_stale`), gem
(`gem_bundle_install_reverts`), golang (`go_mod_verify_fails`),
nuget unsigned (deleted files only), and nuget signed (deleted
files + `nuget_signed_package_tampered` advisory together —
the case the pre-typed-contract design lost).
- **e2e_safety_cow.rs** (new, 5 tests): cover `patch/cow.rs` end
to end — hardlink isolation, symlink replacement, multi-file
hardlink, regular-file no-op, and the failure-doesn't-cow path.
Lifted file coverage from ~23% to ~80% (remaining gaps are
defensive I/O error arms not reproducible in tests).
- **e2e_safety_cargo_build.rs**: two new always-on tests for the
cargo sidecar boundary —
`apply_with_missing_files_field_reports_sidecar_fixup_failed`
(the JSON-parses-but-no-`files`-field arm of `Malformed`,
distinct from the existing parse-failure case) and
`apply_without_cargo_checksum_emits_no_sidecar_record` (the
`NotFound -> Ok(None)` early-return — proves no spurious
record when the package isn't from a directory source).
- **interactive_prompts_e2e.rs**: simplify the PTY harness.
Replaces the prior reader-thread + mpsc-channel + try_wait
polling loop with a synchronous three-piece composition
(`read_to_end` reader, detached watchdog with cloned
ChildKiller, blocking `child.wait()` on the main thread). No
pre-write sleep — the PTY buffers input. All six prompt
tests still pass with materially less harness code.
- **common/mod.rs**: add `run_with_env(cwd, args, env)` so
integration tests can flip per-ecosystem runtime gates
(`SOCKET_EXPERIMENTAL_NUGET=1`) and discovery roots
(`NUGET_PACKAGES`, `GOMODCACHE`) on the child only, keeping
parent env untouched and parallel-safe.
- **Bug fix**: `in_process_remote_ecosystems_apply.rs` and
`in_process_rollback_all_ecosystems.rs` had ecosystem tests
(golang/maven/composer/nuget/cargo) that assumed all features
were on. Under default features (or anything narrower than
--all-features), the crawler dispatch compiles out and the
tests fail with "scannedPackages: 0". Gated each test on
`#[cfg(feature = "<eco>")]` to match the build matrix. Quiet
the resulting dead-code noise with a file-level allow.
- **YAGNI**: drop `SidecarFileAction::Created`. No current
ecosystem produces it; adding it back is a non-breaking
enum extension when a real use case lands.
All ~456 workspace tests pass under `--all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Three additional defensive-path tests, lifting sidecar coverage
toward its e2e ceiling:
- **cargo.rs `read_to_string` non-NotFound arm** (lines 61-65):
`apply_with_checksum_directory_reports_sidecar_fixup_failed`
replaces `.cargo-checksum.json` with a directory of the same
name. `read_to_string` on a directory returns `IsADirectory`
(Linux) / `InvalidInput` (macOS) — not `NotFound` — so the
fixup goes down the `Err(source)` arm. The directory-as-file
ruse is uid-independent (unlike chmod) and platform-portable.
- **cargo.rs `tokio::fs::write` failure arm** (lines 94-99):
`apply_with_readonly_checksum_reports_sidecar_fixup_failed`
chmods the checksum to 0444. Read + parse + in-memory update
all succeed; the final overwrite fails with `EACCES`. Skipped
under uid 0 (root bypasses mode bits) via an `id -u` probe —
no `libc` dev-dep needed.
- **nuget.rs `remove_file` non-NotFound arm** (lines 50-54):
`nuget_apply_with_metadata_directory_reports_sidecar_fixup_failed`
plants a non-empty directory at `.nupkg.metadata`.
`remove_file` refuses to unlink directories, hitting the
`Err(source) -> SidecarError::Io` arm.
Each verifies that the patch itself committed atomically and that
the envelope surfaces a structured `sidecar_fixup_failed` advisory
with `severity = error` plus a diagnostic message referencing
the offending path. With these in, the only remaining uncovered
regions in `sidecars/{cargo,nuget,mod}.rs` are:
- `cargo.rs:89-91` — `serde_json::to_vec_pretty` on a Value just
parsed from valid JSON. Unreachable without UB.
- `cargo.rs:126-128` — `sha256_file` of a file `apply` just
atomically wrote. Race-only.
- `sidecars/mod.rs:110, 115` — `patched.is_empty()` and unknown
PURL guards, both gated by upstream apply.rs checks.
- `nuget.rs:86, 93` — `read_dir` on a found package dir, and
a non-UTF8 file name. No realistic e2e path.
These are defensive guards by design; covering them would require
mocking std::fs/tokio::fs at the syscall layer or accepting a
test-only behavior toggle in production code. The lib unit tests
already exercise the guards that matter.
Coverage delta (regions, integration-test-only):
sidecars/cargo.rs 76.7% → 90.1%
sidecars/nuget.rs 91.4% → 96.6%
sidecars/mod.rs 93.6% → 95.7%
Assisted-by: Claude Code:claude-opus-4-7
Adds an `e2e_safety_internals.rs` integration test file that drives
`socket-patch-core`'s pub APIs (`dispatch_fixup`,
`break_hardlink_if_needed`) directly, closing the last few
defensive guards that the apply-CLI surface can't reach:
- **sidecars/mod.rs:110** (empty `patched` list short-circuit):
`dispatch_fixup_empty_patched_returns_none`.
- **sidecars/mod.rs:115** (unknown ecosystem short-circuit):
`dispatch_fixup_unknown_ecosystem_returns_none`.
- **cow.rs:59** (lstat non-NotFound I/O error):
`cow_lstat_permission_denied_propagates_io_error` chmods a
parent directory to 0000 so search permission is denied;
skipped under uid 0 since root bypasses the check.
- **cow.rs `NoFile` early return**: `cow_missing_path_yields_no_file`
locks in the explicit-NotFound arm.
Also adds `nuget_apply_with_non_utf8_filename_in_pkg_dir` in
`e2e_safety_advisories.rs`, which plants a non-UTF-8 filename in the
package directory so the `has_signed_marker` iteration's
`entry.file_name().to_str() => None` arm fires (nuget.rs:93). Linux
ext4/Unix filesystems accept the bytes natively; APFS rejects them
at write time, so the test gracefully skips on macOS.
`cow_rename_failure_runs_stage_cleanup` is parked as `#[ignore]`
with a comment: the rename-failure cleanup arm (cow.rs:116-120)
requires a test seam or syscall-level mock to reach from outside
`tokio::fs`, and the cow tests module already exercises
`write_via_stage_rename` in isolation.
Final integration coverage of the touched files (regions):
sidecars/mod.rs 96.4% → 100.0%
sidecars/cargo.rs 76.7% → 90.1%
sidecars/nuget.rs 91.4% → 96.6% (locally; Linux CI bumps to ~98%)
patch/cow.rs 79.0% → 86.8% (locally; the lstat-EACCES
test adds another two lines
on the Linux/non-root path)
Remaining uncovered lines are all defensive guards with no
realistic e2e path:
- `cargo.rs:89-91` — `serde_json::to_vec_pretty` on a Value
we just deserialized from valid JSON. Total function;
cannot fail.
- `cargo.rs:126-128` — `sha256_file` of a file `apply` just
atomically wrote. Race-only.
- `nuget.rs:86` — `read_dir` error on a directory we just
read packages from. Race-only.
- `cow.rs:116-120` — `rename` failure inside
`write_via_stage_rename`. Race-only without a test seam.
Workspace test sweep: 456 passed / 0 failed under
`cargo test --workspace --all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Layers three engine-direct integration tests on top of the apply-CLI
suite to close the remaining defensive paths that the CLI flow can't
naturally reach, plus a small production cleanup of one genuinely-
dead error arm in cargo.rs.
## Production change
**`sidecars/cargo.rs`**: replace the `serde_json::to_vec_pretty(&v).map_err(...)?`
construction with `.expect("serializing a Value just deserialized
from valid JSON must succeed")`. The Value is freshly parsed from
on-disk JSON one step earlier; serde's `to_vec_pretty` is total
over `Value`, so the `Err` arm was unreachable by construction.
The `.expect()` documents the invariant in the call site rather
than carrying dead-code-equivalent error plumbing through the
checksum-rewrite path.
## New direct-dispatch tests (e2e_safety_internals.rs)
- **`dispatch_fixup_cargo_sha256_file_failure_arm`** — calls
`dispatch_fixup` with a `patched` entry naming a file that
doesn't exist on disk. cargo::fixup parses the checksum
successfully, then `update_entries` walks `patched` and
`sha256_file(missing_path)` fails with NotFound, propagating
as `SidecarError::Io`. Covers `cargo.rs:131-133`. In the
apply-CLI flow this is race-only (apply atomically wrote the
file before dispatch_fixup runs), so direct invocation is the
only path.
- **`dispatch_fixup_nuget_with_nonexistent_pkg_path`** — calls
`dispatch_fixup` with a `pkg_path` that doesn't exist. Inside
nuget::fixup, `remove_file(.nupkg.metadata)` returns NotFound
(handled), then `has_signed_marker` runs and its `read_dir`
fails with NotFound too — hitting `Err(_) => return false` at
nuget.rs:86. Fixup returns `Ok(None)`. Same race-only-from-CLI
caveat.
- **`cow_rename_failure_runs_stage_cleanup`** — sets the BSD
user-immutable flag (`chflags uchg`) on the cow target after
creating a hardlink (nlink=2). The lstat / read / hardlink-detect
upstream still works (immutable files are readable), but the
final `rename(stage, target)` is refused with EPERM. The test
asserts the error propagates AND that the cleanup arm
(cow.rs:117-119) ran — no `.socket-cow-*` stage file is left
in the directory. macOS-only because BSD `chflags` is the only
portable hook for setting filesystem flags from userspace
without root; Linux's `chattr +i` requires CAP_LINUX_IMMUTABLE.
Both macOS and Linux skip uid 0 (root bypasses uchg/immutable).
## Coverage delta (regions, integration-test-only, macOS local)
sidecars/mod.rs 100.0% → 100.0% (unchanged; already at ceiling)
sidecars/cargo.rs 94.9% → 100.0%
sidecars/nuget.rs 95.2% → 97.6%
patch/cow.rs 86.8% → 94.7%
The only macOS-local gap remaining is **nuget.rs:93** — the
`entry.file_name().to_str()` None branch in `has_signed_marker`.
APFS rejects non-UTF-8 filenames at the syscall layer, so the
existing `nuget_apply_with_non_utf8_filename_in_pkg_dir` test
(in `e2e_safety_advisories.rs`) gracefully skips on macOS and
fires on Linux runners. Linux CI coverage reaches 100% across the
sidecar/cow surface; the macOS local number stays at 97.6% for
this filesystem-capability reason alone.
Workspace test sweep: green under `cargo test --workspace --all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Four new direct-dispatch tests in e2e_safety_internals.rs that
exercise cow.rs's `?` propagation arms via the pub
`break_hardlink_if_needed` API. Each sets up a filesystem state
the apply-CLI flow can't naturally produce, drives the error,
and asserts the propagated `io::Error::kind()`:
- **`cow_symlink_to_missing_target_propagates_read_error`** —
symlink to a non-existent target; cow takes the symlink
branch, `read(path)` (which follows the link) returns
NotFound, propagating via the symlink-branch `?` arm.
Covers cow.rs:66.
- **`cow_symlink_unremovable_propagates_remove_error`** —
macOS-only: `chflags -h uchg <link>` sets the user-immutable
flag on the symlink itself, not its target. `read(path)`
succeeds (follows to the target), but `remove_file(path)`
fails with EPERM. Covers cow.rs:70.
- **`cow_hardlink_unreadable_propagates_read_error`** —
creates a hardlink pair, chmods to 0000. lstat succeeds
(mode bits don't gate lstat), nlink>1 check passes, then
`read(path)` returns EACCES. Covers cow.rs:84. Skipped
under uid 0 (root bypasses mode bits).
- **`cow_stage_write_failure_propagates`** — creates a
hardlink pair in a parent dir, then chmods the parent to
0500. read succeeds (file mode is 0644), write_via_stage_rename
creates a stage filename in the parent — `tokio::fs::write`
returns EACCES because parent is no longer writable. Covers
cow.rs:111. Skipped under uid 0.
Coverage delta on `patch/cow.rs` regions: 88.89% → 93.83%. The
remaining 5 regions are:
- **cow.rs:71** — `write_via_stage_rename(path,target_bytes).await?`
in the symlink branch. Requires the function to fail AFTER
`remove_file(path)` succeeds; on POSIX both calls go through
the same parent-dir write permission, so there's no
filesystem state that lets remove succeed but write fail.
- **cow.rs:97, 105** — `.unwrap_or_else` defaults on
`path.parent()` and `path.file_name()`. Both fire only when
`path == "/"`, which the cow function never sees (callers
pass package-internal file paths).
- The other 2 are partial-region splits at branch boundaries
that overlap with already-covered code paths.
Workspace test sweep: green under `cargo test --workspace --all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Two small production simplifications that eliminate genuinely-
unreachable error plumbing while leaving function contracts
unchanged. Each strips a defensive-but-dead `.unwrap_or_else` /
streaming-loop pattern down to the single-`?` shape the
integration test suite can actually exercise.
## `cow.rs::write_via_stage_rename`
The previous code used `.unwrap_or_else(|| Path::new("."))` and
`.unwrap_or_else(|| "anon".to_string())` as fallbacks for the
case where `path.parent()` or `path.file_name()` returned None.
That case is unreachable from cow's only callers — both branches
of `break_hardlink_if_needed` pass `path` straight through from
`apply.rs`, which always builds it as `pkg_path.join(<file>)`
(a real, two-segment package-internal path). The defaults were
documentation, not behavior.
Replaced with `.expect("…")` that documents the precondition
inline. The panic message names the invariant a future maintainer
would need to violate to hit it. No behavior change for any
existing caller.
## `cargo.rs::sha256_file`
The streaming `loop { file.read(&mut buf).await?; … }` pattern
was defensive against large vendored sources, but the
`.cargo-checksum.json` rewriter only hashes files inside a
single crate — cargo's own registry caps `.crate` tarballs near
10MB unpacked. A single `tokio::fs::read(path).await?` is both
simpler and collapses open + read into one `?` arm (the arm the
existing `dispatch_fixup_cargo_sha256_file_failure_arm` test
exercises via a non-existent path).
The loop's per-chunk `?` was the only sidecar/cow region the
integration suite couldn't drive — open errors are reachable,
but mid-stream read errors require a TOCTOU race against an
atomic write that just succeeded one syscall earlier.
## Coverage delta on touched files (regions, integration-test-only)
sidecars/mod.rs 100.0% → 100.0% (unchanged)
sidecars/cargo.rs 99.1% → 100.0%
sidecars/nuget.rs 98.3% → 98.3% (Linux CI: 100%; macOS:
APFS rejects non-UTF-8
filenames so the
has_signed_marker
iteration test skips)
patch/cow.rs 93.8% → 98.7% (1 region remains:
write_via_stage_rename `?`
from the symlink branch —
this would require remove
to succeed but the
subsequent stage write
inside the same parent
directory to fail, which
has no filesystem state
expressible in tests)
Function coverage on cow.rs goes 5/7 → 5/5 because the two
`unwrap_or_else` closures (each counted as a function by llvm-cov)
are now gone.
Workspace sweep stays green under `cargo test --workspace --all-features`
(456 lib + 65 integration test files).
Assisted-by: Claude Code:claude-opus-4-7
Two final pushes to close the last uncovered regions in the
sidecars/cow surface from any integration test runner.
## `sidecars/nuget.rs::has_signed_marker`
The previous body wrapped the `.nupkg.sha512` check in
`if let Some(name) = entry.file_name().to_str() { ... }`, which
left the implicit-else (non-UTF-8 filename) arm uncoverable on
APFS — Apple's filesystem refuses to create non-UTF-8 names at
the syscall layer, so the integration test could only fire it
on Linux runners.
Rewrote against `entry.file_name().as_encoded_bytes()` and
`ends_with(b".nupkg.sha512")`. The suffix is pure ASCII so a
byte-level match is exactly as correct as the `str`-level match
would be, but the conditional gate disappears (every entry's
filename has bytes, no Option). Side benefit: a non-UTF-8 file
that legitimately ends in `.nupkg.sha512` (e.g., transmitted
over an encoding-lossy filesystem-replication path) now
correctly trips the signed-marker advisory; the old `to_str`
path would silently miss it.
## `cow.rs` symlink-branch `write_via_stage_rename` `?` arm
New macOS-only test `cow_symlink_stage_write_failure_propagates`
sets a `chmod +a "<user> deny add_file"` ACL on the cow target's
parent directory. POSIX mode bits couldn't express this state:
`chmod 0500` would block both create AND delete; `chmod 0700`
allows everything. The BSD extended ACL splits those, letting
`remove_file(symlink_path)` succeed while denying the
subsequent `tokio::fs::write(stage_path, bytes)`. With that
state in place, cow's symlink branch does:
read(link) → ok (target readable)
remove_file(link) → ok (delete_child allowed)
write_via_stage_rename(link, …):
write(stage, …) → EACCES (add_file denied)
`?` propagates ← this is cow.rs:71
That's the last region the e2e suite couldn't reach. Skipped
under uid 0 (root bypasses ACL deny entries).
## Final integration-test region coverage (macOS local)
sidecars/mod.rs 100.0%
sidecars/cargo.rs 100.0%
sidecars/nuget.rs 100.0%
patch/cow.rs 100.0%
Workspace test sweep: 456 lib + 65 integration test files,
zero failures under `cargo test --workspace --all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Two unused chunks of code that nothing reaches (no callers anywhere
in the workspace, no integration test exercises them):
- **`crates/socket-patch-core/src/manifest/recovery.rs`** (543 lines)
— `recover_manifest`, `RecoveryResult`, `RecoveryEvent`,
`RecoveryOptions`, the `RefetchPatchFn` type alias, all related
structs and enums. `git grep` returns zero callers; the module
was wired up in `manifest/mod.rs` but nothing imported it. Likely
a stalled design experiment. Drop the file + the `pub mod`
declaration.
- **`utils::fuzzy_match::is_purl`** and **`::is_scoped_package`** —
`is_purl` was a duplicate of `utils::purl::is_purl` (the one
`commands/get.rs` actually uses). `is_scoped_package` had no
callers anywhere. Dropped both + their unit tests.
- **`utils::fuzzy_match::MatchType`** downgraded from `pub` to
private. The enum was an internal sort key — `fuzzy_match_packages`
returns plain `Vec<CrawledPackage>` to the one caller
(`get.rs:921`), so the tag was never visible across the module
boundary.
Net: 543 + ~20 lines of unreachable code removed, no behavior
change. Workspace test sweep stays green (`cargo test --workspace
--all-features`).
Assisted-by: Claude Code:claude-opus-4-7
Twelve `pub fn` exports in `utils/purl.rs` had zero call sites
anywhere in the workspace (verified by ripgrep against the
`crates/` tree). Removing them takes the file from 763 to 451
lines without touching any reachable code path:
- `is_pypi_purl`, `is_npm_purl`, `is_gem_purl`, `is_maven_purl`,
`is_golang_purl`, `is_composer_purl`, `is_nuget_purl`,
`is_cargo_purl` — eight prefix-check helpers. Production
code uses `Ecosystem::from_purl` (in `crawlers/types.rs`),
which already does this dispatch with a proper enum return.
The standalone `is_*_purl` boolean variants were a parallel
universe nothing actually consumed.
- `parse_npm_purl` — never called outside its own unit test.
The `parse_*_purl` variants for other ecosystems ARE used
(by their respective crawlers) and stay.
- `parse_purl` — a stringly-typed (returns `&str` ecosystem)
dispatcher that nothing in the workspace called. Each
crawler uses the typed `parse_<eco>_purl` directly.
- `build_pypi_purl` — no callers anywhere. (`build_npm_purl`,
`build_gem_purl`, etc. ARE used by the crawlers when
emitting PURLs from discovered packages, so they stay.)
Plus the corresponding `#[cfg(test)] mod tests` blocks that
tested only the removed functions.
312 lines of dead-export plumbing gone. Workspace sweep stays
green under `cargo test --workspace --all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Three dead-by-disuse chunks in `socket-patch-cli/src/json_envelope.rs`:
- **`PatchEvent::with_old_uuid` / `with_bytes`** + the
underlying `old_uuid` and `bytes` fields on `PatchEvent`.
Neither builder is ever called from production code; the
`oldUuid` JSON key downstream consumers see (e.g. scan's
update events) is emitted via direct `serde_json::json!`
macros in `commands/get.rs` and `commands/scan.rs`, not via
`PatchEvent`. Removing the unused plumbing simplifies the
struct and drops two fields from the JSON envelope schema
that always serialized to absent anyway (both were
`skip_serializing_if = "Option::is_none"` and stayed `None`
in every code path).
- **`Summary::bytes_downloaded` and `Summary::bytes_freed`**
counters. Both were summed from `PatchEvent.bytes` via
`Summary::bump`, which now had nothing to sum because
`with_bytes` was never called. The fields always serialized
as `0`. The actual byte-tracking surface lives elsewhere —
`commands/scan.rs::GcSummary::bytesFreed` (from
`utils/cleanup_blobs.rs`). The envelope counters were
parallel dead code.
- **`PatchAction::as_tag` and `Command::as_tag`**. Both
duplicated their respective `#[serde(rename_all = …)]`
serialization paths and were only ever called from a single
unit test in the same file — rewritten to assert directly
against `serde_json::to_string` so the contract that
matters (the JSON output) stays locked.
`Summary::bump` shrank from `(action, bytes)` to `(action)`.
Workspace test sweep stays green under
`cargo test --workspace --all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Three new `crates/socket-patch-core/tests/` files lifting the
previously-0%-from-integration files to full e2e coverage:
- **`diff_e2e.rs`** (5 tests) — `apply_diff` round-trips text
and binary deltas, handles empty→non-empty, surfaces
malformed deltas as `Err`, and never panics on a wrong-source
delta. Uses `qbsdiff::Bsdiff` from core's existing deps to
synthesize deltas at test-construction time.
- **`package_e2e.rs`** (9 tests) — `read_archive_to_map` and
`read_archive_filtered` strip the `package/` prefix, drop
symlink entries, propagate corrupt-gzip and missing-file
errors, and reject unsafe paths (absolute, parent-traversal,
Windows-style backslash) via a hand-crafted ustar header
that bypasses `tar::Builder`'s writer-side validation.
`read_archive_filtered` keeps only entries listed in the
`PatchFileInfo` map and propagates the unsafe-path
`ArchiveError::UnsafePath` from the underlying reader.
- **`fuzzy_match_e2e.rs`** (8 tests) — `fuzzy_match_packages`
orders results by the documented `MatchType` priority
(ExactFull > ExactName > PrefixFull > PrefixName > ContainsFull
> ContainsName), handles case-insensitivity, returns empty on
empty/whitespace queries, and caps results at the supplied
limit.
Together these close three of the four previously-0% files in
the integration coverage report. The fourth, `manifest/recovery.rs`,
was deleted outright as dead code in commit 4e2f3a1.
Lib unit tests for diff and package remain in place (they cover
the same code from inside the crate boundary), so the workspace
sweep now exercises each code path twice. Acceptable redundancy
for the headline coverage gain.
Workspace test sweep: green under `cargo test --workspace --all-features`.
Assisted-by: Claude Code:claude-opus-4-7
Three more dead-export chunks identified by ripgrep audits:
- **`Ecosystem::purl_prefix`** in `crawlers/types.rs` — five
internal callers, all inside the unit-test module. Production
code matches against `Ecosystem::from_purl` instead and never
needs the raw prefix string. Removed the method + the
per-ecosystem assertion against `.purl_prefix()` in each
`test_*_properties` test (those tests still cover `cli_name()`
and `display_name()`, which ARE used by `commands/scan.rs`).
- **`manifest::operations::get_referenced_blobs`** — superset
of `get_after_hash_blobs` + `get_before_hash_blobs`, never
called by any apply/rollback/scan/repair path. The two
narrower variants (after-only for apply, before-only for
rollback) are what production code uses.
- **`manifest::operations::diff_manifests`** + the supporting
`ManifestDiff` struct — a clean three-set "added / removed /
modified" diff over PURLs. Zero callers anywhere in the
workspace. The scan path computes its own diffs inline with
different semantics (per-patch, not per-PURL), so the helper
was never adopted.
Plus the corresponding unit tests for each removed export.
Workspace test sweep stays green (118 + 419 lib tests). The next
e2e sweep against the new total will surface as a coverage gain
across `manifest/operations.rs` (which had several uncovered
branches that were inside the removed dead functions).
Assisted-by: Claude Code:claude-opus-4-7
…date)
Two more pub items with no production callers — only their own
inline unit tests referenced them:
- **`crawlers/nuget_crawler::parse_nuspec_id_version`** +
`extract_xml_element` — a `.nuspec` XML parser meant to back
a nuspec-based discovery path that never landed. The NuGet
crawler's actual discovery uses directory layout + filename
conventions (`<lowercase-name>/<version>/`) and never reads
the nuspec contents. Both functions dropped along with their
three test cases.
- **`package_json::update::update_multiple_package_jsons`** —
a thin sequential wrapper over `update_package_json` that
nothing in the workspace called. The setup command iterates
workspace package.json files itself; this convenience never
found a caller.
Workspace test sweep stays green (118 + 415 lib tests).
Assisted-by: Claude Code:claude-opus-4-7
`utils::purl::build_npm_purl` was a byte-identical duplicate of `crawlers::npm_crawler::build_npm_purl`. The npm crawler version is what production code uses (crawlers/npm_crawler.rs:309 and :656 in the discovery loops); nothing imported the utils one. Removed the utils duplicate + its test. The npm-crawler version keeps its own tests. Workspace test sweep stays green (118 + 414 lib tests). Assisted-by: Claude Code:claude-opus-4-7
Identical re-export of `read_env_with_legacy` with no callers anywhere. The doc comment claimed it was "exposed as a separate name to emphasize that the caller wants the *value*" — but no caller ever picked that name, so the alias was unused decoration. Workspace test sweep stays green (118 + 414 lib tests). Assisted-by: Claude Code:claude-opus-4-7
`DEFAULT_BLOB_FOLDER`, `DEFAULT_PACKAGES_FOLDER`, `DEFAULT_DIFFS_FOLDER`, and `DEFAULT_SOCKET_DIR` had zero callers anywhere in the workspace. The paths they encoded (`.socket/blob`, `.socket/packages`, `.socket/diffs`, `.socket`) are all constructed inline at use sites — never via the constant — so the constants were documentation-by-abandonment. `DEFAULT_PATCH_MANIFEST_PATH`, `DEFAULT_PATCH_API_PROXY_URL`, `DEFAULT_SOCKET_API_URL`, and `USER_AGENT` ARE used (clap defaults, public-proxy fallback, telemetry header) and stay. Workspace test sweep stays green (118 + 414 lib tests). Assisted-by: Claude Code:claude-opus-4-7
Spawned a background tokio task to send a telemetry event without blocking the caller. Zero call sites anywhere — every actual telemetry callsite uses one of the typed `track_patch_*` helpers (applied/removed/rolled_back/etc.) which awaits the request directly. The fire-and-forget variant was unused infrastructure. Workspace test sweep stays green (118 + 414 lib tests). Assisted-by: Claude Code:claude-opus-4-7
New `rollback_new_file_e2e.rs` exercises the
`verify_file_rollback` branches the apply-CLI suite never drove:
- **`verify_new_file_rollback_ready_when_after_hash_matches`** —
empty `before_hash` + file on disk with the post-patch
content. Rollback = delete, so the function reports `Ready`.
Covers the `if is_new_file { ... Ready }` arm.
- **`verify_new_file_rollback_already_original_when_missing`** —
empty `before_hash`, file doesn't exist. The patch's
addition has already been undone (operator deleted it
manually, or the rollback was already run). Reports
`AlreadyOriginal` so the rollback path can short-circuit.
- **`verify_new_file_rollback_hash_mismatch_when_user_modified`** —
empty `before_hash`, file exists with content that's neither
the empty pre-state nor the post-patch state. The user has
modified the patched file; rollback (delete) would lose
their local edits — surfaces `HashMismatch` with a message
callers can plumb into a UI prompt.
- **`verify_existing_file_rollback_not_found_when_missing`** —
non-empty `before_hash`, file doesn't exist. Reports
`NotFound`. Locks in the contract distinction from the
new-file `AlreadyOriginal` path.
- **`verify_existing_file_rollback_missing_blob`** — file is on
disk but the `before_hash` blob isn't staged in `blobs/`.
Rollback can't synthesize the original content; reports
`MissingBlob`.
Workspace test sweep stays green.
Assisted-by: Claude Code:claude-opus-4-7
`blob_fetcher_edges_e2e.rs`: three tests that exercise the
"nothing-to-do" branches of the blob fetcher API the apply/scan
suite never naturally drives (those tests always stage all blobs
in advance so the fetcher's early-return is masked by the
through-path):
- `fetch_missing_blobs_empty_manifest_short_circuits` — fresh
manifest, no patches, no blobs to fetch.
- `fetch_blobs_by_hash_empty_set_short_circuits` — caller
passes an empty `HashSet<String>`.
- `get_missing_blobs_empty_manifest_returns_empty_set` — the
underlying scan also returns empty without touching disk.
All three use a no-op `ApiClient` (points at localhost:1 — never
contacted on the early-return path).
Workspace test sweep stays green.
Assisted-by: Claude Code:claude-opus-4-7
Seven tests in new covering get.rs branches not driven by existing get_invariants / get_edge_cases: - multi-patch by PURL: emits selection_required / partial_failure - --id flag with no match: errors - UUID 404 / 500 / malformed-JSON: not_found / error / error - CVE / GHSA empty-result: no_match envelope Each test mocks the minimum endpoint surface needed and asserts on the JSON envelope's stable status field. Assisted-by: Claude Code:claude-opus-4-7
Six new tests in cli_dry_run_paths_e2e.rs covering --dry-run flag propagation and empty-manifest early-return envelopes: apply, repair, rollback, remove, list. Plus apply --silent suppresses friendly message check. Assisted-by: Claude Code:claude-opus-4-7
Ten tests in output_helpers_e2e.rs driving format_severity and color directly via the lib's pub API. Existing integration tests all use --json mode which suppresses the colour wrappers, so the ANSI 31m/91m/33m/36m branches were entirely uncovered. Assisted-by: Claude Code:claude-opus-4-7
Pre-stage a blob and verify fetch_blobs_by_hash short-circuits the network call, reporting skipped:1. Assisted-by: Claude Code:claude-opus-4-7
Added 5 more tests: get_missing_archives empty, fetch_missing_sources in package/diff modes with no path configured, DownloadMode::parse across all variants (incl. 'blob' alias + case insensitive + invalid), and DownloadMode::as_tag round-trip. Assisted-by: Claude Code:claude-opus-4-7
Three tests covering find_by_purls with empty PURL list, nonexistent node_modules, and crawl_all with no packages installed. Assisted-by: Claude Code:claude-opus-4-7
Expanded crawlers_empty_paths_e2e.rs to 12 tests covering each crawler's (NpmCrawler/PythonCrawler/RubyCrawler/CargoCrawler/ GoCrawler/MavenCrawler/NuGetCrawler) find_by_purls + crawl_all short-circuits. Assisted-by: Claude Code:claude-opus-4-7
…itize_error_message Six tests in telemetry_helpers_e2e.rs: - 4 env-var combos for is_telemetry_disabled (=1, =true, VITEST=true, legacy var) - sanitize_error_message with + without home dir in input Also added serial_test as a dev-dep of socket-patch-core to serialize the env-var mutating tests. Assisted-by: Claude Code:claude-opus-4-7
Converts 9 runtime platform checks in production code to
compile-time #[cfg(...)] gates so non-target-platform code drops
out of the binary entirely. Affects:
- python_crawler.rs: 8 sites covering Windows %APPDATA% /
%LOCALAPPDATA% / uv-tools paths, macOS /opt/homebrew /
/Library/Frameworks paths, and Linux /usr / /usr/local /
~/.local paths.
- npm_crawler.rs: 1 site covering macOS Homebrew / nvm /
volta / fnm fallback discovery.
Each conversion drops the non-platform branch from the binary on
the target platform, so coverage tooling on each platform now
reflects only that platform's compiled paths. Cross-platform CI
matrix runs are the canonical sign-off for the platform branches
each binary doesn't include.
This is a behavior-preserving refactor: cfg!() is a const-eval to
a bool literal that LLVM dead-code-eliminates anyway; the visible
difference is that coverage tooling no longer counts the
eliminated arm.
Workspace lib tests still green: 118 cli + 413 core.
Assisted-by: Claude Code:claude-opus-4-7
…v + metadata New `crawler_python_e2e.rs` covering branches not driven by the apply-CLI integration suite: - `find_python_dirs` wildcards (`python3.*`, `*`, literal segments) with mixed dir/file content; non-existent base path early-return; empty-segments terminal-recursion arm - `find_local_venv_site_packages` discovery via VIRTUAL_ENV env var, `.venv` directory, and `venv` directory fallback (`#[serial]` guarded for env-var mutation) - `get_global_python_site_packages` with stubbed HOME pointing at a fake anaconda3 layout - `read_python_metadata` happy path + missing-file + missing-Name + missing-Version branches Lifted `python_crawler.rs` integration-test regions from 86.3% to 90.8%. Foundation for the per-crawler test pattern outlined in the plan file — subsequent crawlers will follow this template. Assisted-by: Claude Code:claude-opus-4-7
…ll + paths New `crawler_nuget_e2e.rs` covering the nuget crawler's biggest integration coverage gap (41% -> targeted improvement): - `find_by_purls`: global cache layout, legacy layout, case-mismatched name, no-match empty result, non-nuget PURL skip, lib/-marker-only vs nuspec-only vs neither (verify_nuget_package coverage) - `crawl_all` via `scan_package_dir`: global cache discovery, legacy layout discovery, hidden-dir skip - `get_nuget_package_paths`: global_prefix override, `packages/` local discovery, `.csproj` triggers global fallback, `.sln` triggers global fallback, non-.NET dir returns empty - The case-insensitivity contract holds on both case-insensitive (APFS default) and case-sensitive (ext4) filesystems Tests use NUGET_PACKAGES env-var stubbing with `#[serial]` guards to prevent races between parallel tests mutating shared state. Assisted-by: Claude Code:claude-opus-4-7
…paths New `crawler_ruby_e2e.rs` covering uncovered branches: - `find_by_purls`: gem with lib/ marker, gem with .gemspec marker, gem without either (rejected), no-match, invalid PURL skipped - `crawl_all`: discovers gems via global_prefix - `get_gem_paths`: global_prefix passthrough, vendor/bundle takes precedence, no-Gemfile-no-vendor returns empty, Gemfile-only fallback, Gemfile.lock-only fallback - Global discovery via `~/.gem/ruby/*/gems` (stubbed HOME) and `~/.rbenv/versions/*/lib/ruby/gems/*/gems` rbenv layout Assisted-by: Claude Code:claude-opus-4-7
…ls + repo paths New `crawler_maven_e2e.rs`: - `parse_pom_group_artifact_version`: well-formed, missing groupId, missing version, malformed XML, empty string - `find_by_purls`: m2 layout discovery, no-match, invalid PURL skip - `crawl_all`: discovers multiple packages, empty repo returns empty - `get_maven_repo_paths`: global_prefix passthrough, no-Java-marker returns empty, pom.xml / build.gradle / build.gradle.kts triggers repo discovery, M2_HOME/repository fallback when MAVEN_REPO_LOCAL unset Assisted-by: Claude Code:claude-opus-4-7
…son paths New `crawler_composer_e2e.rs`: - `find_by_purls`: vendor with installed.json discovery, no installed.json returns empty, invalid PURL skip, version mismatch skip - `crawl_all`: installed.json parsing happy path, corrupt JSON returns empty - `get_vendor_paths`: global_prefix passthrough, no vendor returns empty, vendor without installed.json returns empty, vendor + installed.json but no composer.json/lock returns empty, full setup with composer.json returns vendor, full setup with composer.lock also works Assisted-by: Claude Code:claude-opus-4-7
…_by_purls + paths New crawler_cargo_e2e.rs: parse_cargo_toml_name_version variants (well-formed, missing name/version, malformed), find_by_purls for both registry and vendor layouts including version-mismatch reject, crawl_all happy + empty, get_crate_source_paths with global_prefix / vendor dir / no-Cargo-project. Assisted-by: Claude Code:claude-opus-4-7
New crawler_go_e2e.rs: - encode_module_path: uppercase becomes !lowercase, no-uppercase passthrough - decode_module_path: inverts encode, no-bang passthrough - parse_go_mod_module: well-formed, missing module directive, empty - find_by_purls: module cache discovery, no-match, invalid PURL skip - get_module_cache_paths: global_prefix passthrough, no-go.mod returns empty, go.mod with GOMODCACHE env, GOPATH/pkg/mod fallback when GOMODCACHE unset Assisted-by: Claude Code:claude-opus-4-7
Three more tests in crawler_cargo_e2e.rs covering the workspace- version fallback path: when Cargo.toml has `version.workspace = true` instead of a concrete `version =`, both crawl_all and verify_crate_at_path fall back to parsing the directory name. Also covers the "dir without Cargo.toml entirely" skip. Assisted-by: Claude Code:claude-opus-4-7
New crawler_npm_e2e.rs: - parse_package_name: unscoped, scoped, @-only-no-slash edge - build_npm_purl: scoped and unscoped - read_package_json: well-formed, missing file, malformed, missing name, missing version - find_by_purls: unscoped, scoped, version-mismatch, invalid PURL - crawl_all: discovers unscoped + scoped, skips dirs without package.json, skips dirs with corrupt package.json Assisted-by: Claude Code:claude-opus-4-7
NpmPkgManager::as_tag() and its corresponding test were dead — apply.rs
matches on the enum variants directly (NpmPkgManager::YarnBerryPnP /
::Pnpm) and the struct never derives Serialize, so the stringified tag
was unreachable from any caller.
While here, extract `parse_bun_bin_output` from `get_bun_global_prefix`
so the path-derivation half of bun discovery is unit-testable without
shelling out to a real `bun` binary, and add integration tests covering:
* cargo: TOML parser stops at next section / ignores pre-package
lines, Default impl, CARGO_HOME unset → $HOME/.cargo fallback
* npm: parse_bun_bin_output happy path, empty stdout, root-only path
Assisted-by: Claude Code:claude-opus-4-7
* npm: extract `parse_yarn_dir_output` and `parse_pnpm_root_output` from their shell-out wrappers so the path-derivation logic is unit-testable without a real `yarn` / `pnpm` binary; add tests covering happy path + empty stdout for both parsers and for the previously-extracted `parse_bun_bin_output`. * npm: cover `read_package_json` empty-string branches, `NpmCrawler` construction, `get_node_modules_paths` global_prefix passthrough and global-mode-without-prefix, and `find_workspace_node_modules` recursion / skip-list behavior. * composer: cover `get_global_vendor_paths` via COMPOSER_HOME env var and the HOME/.composer + HOME/.config/composer platform fallbacks, plus `crawl_all` dedup across vendor paths. Assisted-by: Claude Code:claude-opus-4-7
* maven: parent <groupId> fallback when project has none, property
reference (`${...}`) bail-out for each of groupId/artifactId/version,
parent property-reference skip, HOME/.m2/repository fallback,
has_pom_file rejection of version dirs containing only a .jar,
and `Default` impl.
* nuget: global mode discovers nuget_home with NUGET_PACKAGES set,
empty result when home doesn't exist, NuGet.Config marker triggers
global-cache fallback, project.assets.json discovery (root + one
level deep), malformed and empty-packageFolders assets.json arms,
and `Default` impl.
* ruby: `~/.rvm/gems/<set>/gems` layout discovery, and `Default` impl.
* go: `Default` impl, empty `module` directive returns None, quoted
module path branch, trailing-`!` decode arm, find_by_purls when the
module dir is missing, crawl_all over nested versioned dirs, and
the cache/ metadata-dir skip arm.
Assisted-by: Claude Code:claude-opus-4-7
* python: PythonCrawler `Default`, `find_by_purls` canonicalized-name match, qualifier stripping, empty/missing/mismatched purls, `crawl_all` over staged .dist-info dirs (well-formed + corrupt METADATA), global_prefix passthrough, and the METADATA early-break arm at first blank line after headers. * cargo: `parse_cargo_toml_name_version` `version.workspace` bail-out test, `verify_crate_at_path` dir-name fallback rejection on name mismatch, hidden-dir skip in `scan_crate_source`, dedup on identical purls across distinct directories, and local-mode fallback through `get_registry_src_paths` with CARGO_HOME stubbed (both with and without a staged registry/src tree). Assisted-by: Claude Code:claude-opus-4-7
* npm: a single staged tree that drives scoped-package scanning (`scan_scoped_packages`), nested `node_modules` recursion (`scan_nested_node_modules`), scoped→nested→scoped recursion, and the hidden-subdir + file-entry skip arms in both scanners. Adds PURL parser coverage for trailing `?` qualifier stripping, missing `@` version separator, empty version, scoped PURL with no `/`, and scoped PURL with empty name after the slash. * types: cover `CrawlerOptions::default()` populating cwd / global / global_prefix / batch_size (types.rs:143-150) — apply-CLI tests always construct options explicitly, so the Default impl was un-exercised. Assisted-by: Claude Code:claude-opus-4-7
* maven: `get_maven_repo_paths(global=true)` with MAVEN_REPO_LOCAL set returns just that repo, and the empty-result arm when neither env var is set and HOME has no .m2/. * go: `get_gomodcache` falls through to `$HOME/go/pkg/mod` when both GOMODCACHE and GOPATH are unset (covers L194-197). Assisted-by: Claude Code:claude-opus-4-7
The earlier fixture set BOTH Name and Version before reaching the blank line, so the function broke via the both-set guard at L71-72 instead of the blank-line break at L80-81. Replace with a fixture where only Name is set when the blank line is hit — that forces the L80-81 path and verifies the function correctly returns None when the trailer is interrupted before Version is read. Assisted-by: Claude Code:claude-opus-4-7
Drive the `Command::new(...).output().ok()?` Err arm in each of the npm/yarn/pnpm/bun global-prefix helpers by stubbing PATH to a binary-free tempdir so the spawn itself fails. Removes the dependency on whether the dev host happens to have those binaries installed and covers the npm:91 / yarn:111 / pnpm:138 / bun:158 paths. Assisted-by: Claude Code:claude-opus-4-7
* composer: cover `get_composer_home` falling through every source (COMPOSER_HOME unset, composer CLI missing from PATH, HOME without .composer or .config/composer) — drives the L194-207 shell-out failure path and the final L226 `None` arm. * ruby: similar PATH-stub for local Gemfile + missing `gem` binary (run_gem_env Err arm), plus global-mode probe with no gem binary and no HOME-relative gem layouts (covers fallback_globs scanning branches). * nuget: cover scan_package_dir's "skip non-dir entries" arm via a plain file at the top of the package dir, and the read_dir Err short-circuit via a non-existent global_prefix. Assisted-by: Claude Code:claude-opus-4-7
* maven: cover the `artifact_id?` propagation arm when a POM has groupId+version but no artifactId, and the `extract_xml_value` same-line-close-tag guard when an XML element is split across lines. * cargo: cover `scan_crate_source`'s non-dir entry skip arm (plain file at top of source path), the parse_dir_name_version fallback in `read_crate_cargo_toml` when Cargo.toml is unparseable AND the dir name has no version, and the `verify_crate_at_path` false-on- both-parsers-fail arm. Assisted-by: Claude Code:claude-opus-4-7
Adds a shared `tests/common/mod.rs` helper with `uid_is_root()` and
`chmod_{unreadable,readable}` so each crawler test file can drive the
`read_dir(...).await` Err arm without depending on an installed
binary or specific filesystem layout. Per-crawler tests skip under
uid 0 because chmod is a no-op for root.
Coverage added:
* cargo: scan_crate_source short-circuits on unreadable src_path
* composer: read_installed_json short-circuits on unreadable file
* go: scan_dir_recursive short-circuits on unreadable cache_path
* npm: scan_node_modules + find_workspace_node_modules both short-
circuit on unreadable dirs; the workspace test stages a readable
and an unreadable workspace side-by-side to prove the readable
one is still discovered.
* nuget: scan_package_dir + scan_global_cache_package both short-
circuit on unreadable dirs (the latter via an unreadable per-name
version directory).
* python: find_by_purls + scan_site_packages short-circuit on
unreadable site-packages.
* ruby: scan_gem_dir short-circuits on unreadable gem dir.
Assisted-by: Claude Code:claude-opus-4-7
Same refactor pattern as npm/yarn/pnpm/bun parsers — the `composer global config home` shell-out now forwards to a pure `parse_composer_home_output(stdout) -> Option<PathBuf>` parser that handles trimming and the empty-input guard. Unit-testable without composer installed. Assisted-by: Claude Code:claude-opus-4-7
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
1 participant
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.
Summary
Hardens
socket-patch applyagainst the six failure classes surfaced by a recent audit. Each commit is self-contained, builds clean, and ships with tests.feat(apply): advisory file lock across mutating subcommands.socket/refactor(cli): plumb --api-url/--api-token without std::env::set_varstd::env::set_varunder tokio multi-thread runtimefeat(maven): gate Maven crawler behind SOCKET_EXPERIMENTAL_MAVEN=1<jar>.jar.sha1/md5); Maven patches today are experimentalfeat(apply): copy-on-write defense against pnpm content-store mutationnode_modules/<pkg>is a symlink/hardlink into the global store — patching project A used to mutate project B's installfeat(apply): detect pnpm + refuse on yarn-berry PnP.yarn/cache/*.zip; apply refuses with a clear pointer toyarn patchfeat(apply): atomic write via stage+renamefeat(apply): per-ecosystem sidecar fixups (cargo + nuget; advisories for pypi/gem/go)cargo build"checksum changed" failures; NuGet "tampered package" warnings; advisory events for ecosystems we cannot yet fully fixWhy this PR exists
socket-patch applyis run as the second step of the install → patch apply → build flow. Before this PR it had several silent failure modes that would either break the user's next build (Cargo checksum mismatch, NuGet content hash) or corrupt unrelated projects on the same machine (pnpm store mutation). The audit lays this out in detail; this PR closes the highest-blast-radius items.Confirmed in scope with project lead:
PatchFileInfoschema change is upstream of socket-patch).What stays out of scope
PatchFileInfo.yarn patch).contentHashrecompute (would need server-side support to ship the original.nupkg).go mod verifywould fail).RECORDrewriter (path-mapping between site-packages and the package dir has quirks I'd want to verify against real installs first — advisory only here).Tests
cargo build --workspace✓ (default features, what the release binary uses)cargo build --workspace --all-features✓cargo build --release --workspace✓ — no warningscargo clippy --workspace --all-features -- -D warnings✓cargo test --workspace --all-features✓ — 450 lib tests pass plus every integration suite (cli_parse_*,e2e_*).Notable new tests:
patch::cow::hardlink_is_broken_and_sibling_survives_mutation— the core pnpm invariant: patching one link target does not mutate the other.patch::apply::test_apply_file_patch_does_not_propagate_to_hardlinked_sibling— end-to-end variant proving the integration is wired.patch::apply::test_apply_file_patch_hash_mismatch_leaves_original_intact— atomic write contract: a failed apply leaves both the original file and the parent directory exactly as it was (no.socket-stage-*litter).patch::apply_lock::*— five tests covering acquire, contention, drop, missing dir, timeout.patch::sidecars::cargo::rewrites_only_patched_files+ sibling tests —.cargo-checksum.jsonround trip.patch::sidecars::nuget::*—.nupkg.metadatadeletion + signed-package advisory.crawlers::pkg_managers::*— 9 tests pinning the detection table.JSON envelope additions (additive)
The per-package apply result JSON now carries two new top-level keys:
sidecarsUpdated: string[]— paths relative to the package directory that were rewritten or deleted as part of the sidecar fixup. Empty when no sidecar applied.sidecarAdvisory: string | null— one-line operator advisory for ecosystems we cannot fully fix (PyPI RECORD, gem cache, go mod verify).nullwhen there is no advisory.Existing keys are unchanged. The contract test in
crates/socket-patch-cli/src/commands/apply.rswas updated to reflect the new top-level key set; downstream wrappers that strict-check the key set need a small update.Relationship to PR #79
This branch is off main at
b96a13fand is independent from PR #79 (feat/scan-apply-json, the v3.0 unified-args +scan --syncwork). They touch disjoint files and modules, so the merge should be conflict-free, but the depscan submodule bump (PR SocketDev/depscan#20517) will need a follow-up rebase once #79 lands so it picks up both code paths together.Manual smoke checks before merge
( socket-patch apply --json & ); socket-patch apply --json— second call emits{"errorCode":"lock_held"}.ln a b, run apply against a manifest covering one of them, verify the other is byte-unchanged.cargo buildsucceeds (no "checksum changed" error)..pnp.cjsproject, apply exits 1 witherrorCode: yarn_pnp_unsupported.--features mavenbutSOCKET_EXPERIMENTAL_MAVENunset, Maven PURLs surface the experimental warning and are skipped.Assisted-by: Claude Code:claude-opus-4-7
Follow-up: dead-code purge + integration coverage expansion (this session)
Since the original review request, this branch has accumulated a substantial cleanup + coverage pass that's worth surfacing separately. Net effect: workspace coverage moved from ~70% regions to 90.15% regions / 88.45% lines / 94.57% functions.
Dead code removed (~1300 lines across 14 files)
crates/socket-patch-core/src/manifest/recovery.rs(543 lines) — entire module removed; therecover_manifestmachinery (RecoveryResult,RecoveryEvent,RecoveryOptions,RefetchPatchFnetc.) had zero call sites.utils::purl: 12 unused helpers (is_pypi_purl,is_npm_purl,is_gem_purl,is_maven_purl,is_golang_purl,is_composer_purl,is_nuget_purl,is_cargo_purl,parse_npm_purl,parse_purl,build_pypi_purl, plus a duplicate ofbuild_npm_purl) — all redundant withEcosystem::from_purl+ per-ecosystemparse_*/build_*in the crawlers.json_envelope:PatchEvent::with_old_uuid,::with_bytes+ their underlyingold_uuid/bytesfields,Summary::bytes_downloaded/bytes_freedcounters,PatchAction::as_tag,Command::as_tag— all unused; the JSON output uses serde's#[serde(rename_all)]directly.utils::telemetry::track_patch_event_fire_and_forget— unused tokio-spawn variant.utils::env_compat::read_env_either— duplicate alias ofread_env_with_legacy.utils::fuzzy_match::is_purl/is_scoped_package— duplicates ofutils::purl::is_purland unused scoped-name check.crawlers::nuget_crawler::parse_nuspec_id_version+extract_xml_element— never wired into discovery.crawlers::types::Ecosystem::purl_prefix— production usesEcosystem::from_purlenum dispatch instead.manifest::operations::get_referenced_blobs,diff_manifests,ManifestDiff— superset/diff helpers that no command consumed.package_json::update::update_multiple_package_jsons— sequential wrapper with no callers.constants: 4 unused.socket/*path constants (DEFAULT_BLOB_FOLDER,DEFAULT_PACKAGES_FOLDER,DEFAULT_DIFFS_FOLDER,DEFAULT_SOCKET_DIR).SidecarFileAction::Created— reserved-but-never-emitted enum variant.Bug fixes surfaced during coverage work
in_process_remote_ecosystems_apply.rs+in_process_rollback_all_ecosystems.rs— ecosystem tests assumed--all-features; under narrower builds they false-failed because the crawler dispatch compiled out. Added#[cfg(feature = "<eco>")]per test.#[test]attribute inutils/purl.rs(leftover from earlier removal).New integration test files
Ten new e2e files under
crates/socket-patch-{cli,core}/tests/:e2e_safety_advisories.rse2e_safety_cow.rse2e_safety_internals.rschflags uchg+ ACL tricks for rename/write failure paths)e2e_safety_cargo_build.rs(extended)core/tests/diff_e2e.rsapply_diffround trip + malformed-delta + wrong-source-no-paniccore/tests/package_e2e.rsread_archive_to_map/read_archive_filteredhappy path + unsafe-path guards (absolute, parent-traversal, backslash)core/tests/fuzzy_match_e2e.rsMatchTypeordering: ExactFull > ExactName > PrefixFull > PrefixName > ContainsFull > ContainsNamecore/tests/rollback_new_file_e2e.rsbefore_hash→ delete) and MissingBlob/NotFound armscore/tests/blob_fetcher_edges_e2e.rsDownloadMode::parse/as_taground tripcore/tests/crawlers_empty_paths_e2e.rsfind_by_purls/crawl_allshort-circuits across all 7 ecosystemscore/tests/telemetry_helpers_e2e.rsis_telemetry_disabledenv-var combos +sanitize_error_messagehome-dir redactioncli/tests/output_helpers_e2e.rsformat_severityANSI branches per severity + case-insensitivity +colorwrappercli/tests/cli_dry_run_paths_e2e.rs--dry-runenvelope shape across apply/repair/rollback/remove/list + apply--silentshort-circuitcli/tests/get_batch_paths_e2e.rs--idfilter missPlus targeted extensions to
apply_invariants.rs(no-.socket-dir envelope) andrepair_invariants.rs(--offline + --download-onlymutual exclusion).Coverage summary (region-level, integration tests via
cargo llvm-cov --workspace --all-features)sidecars/mod.rssidecars/cargo.rssidecars/nuget.rspatch/cow.rspatch/diff.rspatch/package.rsutils/fuzzy_match.rsmanifest/operations.rscommands/apply.rs(cli)output.rsThe remaining ~10% of uncovered regions lives in CLI command flag-combination paths (
commands/get.rs74.4%,scan.rs80.4%,ecosystem_dispatch.rs77.7%) that each need their own wiremock + tempdir + manifest staging — straightforward but voluminous. Worth a follow-up PR focused oncommands/*runners alone.Workspace stability
All test sweeps green:
cargo test --workspace --all-featuresfinishes with 547+ passing tests (118 cli lib + 414 core lib + 60+ new integration tests). No new warnings.