Skip to content

Commit 431b818

Browse files
Fix stale docs and comments, and the npm wrapper's schema and exit code (#1022)
* Correct two false CLI_CONTRACT claims The rollback prompt's decline line is the shared "Cancelled; no changes made." (ui::CANCELLED), not "Rollback cancelled.". The JSON envelope shape is pinned by json_envelope.rs's unit tests and the commands' e2e tests; tests/cli_parse_*.rs pin the parsed clap arguments, not the envelope. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Drop comments describing the deleted hosted ledger and replay v5 hosted mode keeps no ledger, and patch/redirect has no ledger, replay or takeover module any more. The hosted engine, the CLI hosted flow and the staged I/O layer still described a ledger load, a merge-then-persist, a ledger-record fallback for the gem stale-install probe and edit ordering for a whole-ledger replay. Say what the code does now: the vendored ledger is the only one the hosted flow loads, the gem probe judges only this run's fetched records, and staged.rs serves the hosted -> upstream restore. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Fix three stale code comments - update/release.rs: the API client is no longer timeout-free; contrast self-update's whole-request deadlines with ApiTimeouts' connect and per-read bounds instead. - vex/discover/pypi_other.rs: is_socket_hosted_reference was deleted by #572; name hosted_pypi_reference, which shares the path parser and the origin allowlist. - scan/policy.rs: dir_markers falls back to manifests when a directory has no lock marker, which the in-memory engine does not do. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Report a signal-killed CLI as 128 + signal in the npm wrapper spawnSync gives status null when the binary dies by a signal, and the wrapper turned that into exit 1, so Ctrl-C read as an ordinary failure. Return 128 + the signal number, as a shell does (SIGINT 130, SIGTERM 143). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Keep the manifest setup block in the npm schema and run its tests - The zod PatchManifestSchema lacked the legacy setup block the Rust manifest still parses and keeps, so validating a pre-v5 manifest stripped it. Add SetupConfigSchema (optional exclude/manual lists). - package.json had no files list, so the tarball shipped src/, the tsconfig and the compiled tests. Publish only the wrapper, bundled binaries and the compiled schema; a pack test guards the list. - The test script called pnpm in an npm-locked package and no workflow ran it. Use npm, and run it in the dispatch-tests job. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Drop the hosted ledger story from the stale-install probe tests The test helper and one test still described the merged ledger map the deleted hosted ledger used to feed. Production now hands over only this run's fetched records; the renamed test pins what it really checks: a record keyed under another purl spelling still matches by uuid. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Require the compiled schema in the npm tarball once it is built The pack-contents test only allowlisted dist/schema files, so a files list that dropped them would still pass. When dist/schema is built, both manifest-schema.js and .d.ts must be in the pack list. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Rerun the npm pack test after the schema build in CI Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 4f52545 commit 431b818

13 files changed

Lines changed: 180 additions & 69 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -800,6 +800,19 @@ jobs:
800800
- name: Run npm dispatch tests
801801
run: node --test npm/socket-patch/bin/socket-patch.test.mjs
802802

803+
- name: Run npm schema tests
804+
# The `./schema` export's zod tests. `npm install --no-save`, not
805+
# `npm ci`, for the reason publish-npm.yml gives (a version-synced
806+
# lock can name platform packages not yet on the registry); the
807+
# platform binaries are optional and not needed here. The pack
808+
# test runs again once `npm test` has built dist/, so it also
809+
# checks that the compiled schema ships.
810+
working-directory: npm/socket-patch
811+
run: |
812+
npm install --no-save --ignore-scripts --no-audit --no-fund --omit=optional
813+
npm test
814+
node --test --test-name-pattern="npm package contents" bin/socket-patch.test.mjs
815+
803816
# Compiles the CLI and every CLI test target once per OS (--all-features,
804817
# so this is also the feature-gated suites' compile-rot check) and uploads
805818
# the binaries the e2e, e2e-full and cargo-vex legs run. The legs run the

‎crates/socket-patch-cli/CLI_CONTRACT.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -907,7 +907,7 @@ A bare `rollback` (or a scoped one, for its scope) restores the SYSTEM to unpatc
907907
5. **Manifest cleanup** — entries are removed ONLY for in-scope purls whose legs fully succeeded, were not-installed, or were release-variant siblings narrowed away by an attempted variant that succeeded (half a variant group never lingers — `remove` parity); drift-kept and failed purls keep their records, and a failed variant holds its whole group. A manifest record that a live hosted pin has superseded (the lockfile wires the same package release to a different patch uuid, e.g. an agent → hosted migration after the patch was replaced) and whose installed copy holds neither side of the record is not restored in place and does not fail the run: the hosted leg's lock restore and the next install unwind it, and the record leaves the manifest with the `rollback_record_superseded` warning (`remove` does the same instead of aborting before its hosted leg). A copy still holding the record's patched bytes is restored as usual. No-op removals never rewrite the file. A failed write surfaces as `manifest_write_failed` (warning + `partial_failure` exit 1; GC still runs against the unchanged manifest).
908908
6. **GC** — blob, diff and legacy package-archive sweeps against the post-removal manifest, using the same artifact-reference policy as `remove`, retaining beforeHash blobs for (a) removed-but-not-installed entries (a crawler miss must not destroy the only local revert data — `remove` parity) and (b) EVERY entry remaining in the post-removal manifest — still-active patches (failed, drift-kept, eco-/path-excluded) keep their revert data, so a scoped or failed run never destroys the blobs a later rollback needs; only blobs referenced solely by genuinely-removed entries are swept. GC errors warn (`cleanup_failed`) and continue — they never affect the exit (repair's posture).
909909

910-
**Confirmation prompt.** A wet, non-preserve run with work prompts once, remove-style, composing only the clauses that apply into one English list (`a and b`, `a, b, and c`) with counted nouns: `Roll back N patches`, `remove them from the local manifest`, `delete M vendored artifacts and their ledger records`, `restore H hosted packages to the upstream registry` (e.g. `Roll back 1 patch, remove it from the local manifest, and restore 1 hosted package to the upstream registry?`) — default yes, auto-accepted under `--yes`/`--json`/non-TTY (the shared `confirm` semantics; CI unaffected). Decline prints `Rollback cancelled.` and exits 0. `--dry-run` and `--preserve-state` runs are prompt-free (they delete no local state).
910+
**Confirmation prompt.** A wet, non-preserve run with work prompts once, remove-style, composing only the clauses that apply into one English list (`a and b`, `a, b, and c`) with counted nouns: `Roll back N patches`, `remove them from the local manifest`, `delete M vendored artifacts and their ledger records`, `restore H hosted packages to the upstream registry` (e.g. `Roll back 1 patch, remove it from the local manifest, and restore 1 hosted package to the upstream registry?`) — default yes, auto-accepted under `--yes`/`--json`/non-TTY (the shared `confirm` semantics; CI unaffected). Decline prints `Cancelled; no changes made.` (stdout) and exits 0. `--dry-run` and `--preserve-state` runs are prompt-free (they delete no local state).
911911

912912
### `--preserve-state` (opt-out, both `rollback` and `remove`)
913913

@@ -1136,7 +1136,7 @@ The v3.0 legacy names `SOCKET_PATCH_PROXY_URL`, `SOCKET_PATCH_DEBUG` and `SOCKET
11361136

11371137
## JSON output shapes
11381138

1139-
Every `--json` invocation emits a single JSON object that follows the **unified envelope** below. The envelope was introduced in v3.0; older per-command shapes are deprecated. See `src/json_envelope.rs` for the source of truth and `tests/cli_parse_*.rs` for snapshot tests that lock the shape.
1139+
Every `--json` invocation emits a single JSON object that follows the **unified envelope** below. The envelope was introduced in v3.0; older per-command shapes are deprecated. See `src/json_envelope.rs` for the source of truth; its unit tests pin the serialized names, and each command's e2e tests assert the envelope it emits. The `tests/cli_parse_*.rs` files pin the parsed clap arguments, not this shape (a few, such as `cli_parse_list.rs`, also spot-check `list`'s envelope).
11401140

11411141
### Envelope shape
11421142

‎crates/socket-patch-cli/src/commands/scan/hosted.rs‎

Lines changed: 35 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,9 @@ fn refuse(
137137
}
138138

139139
/// The apply lock for a WET hosted run: the same `<manifest dir>/apply.lock`
140-
/// `apply`/`rollback`/`remove`/`vendor` hold, so the takeover pre-reverts,
141-
/// the ledger merge and the lockfile writes never race them. `acquire`
140+
/// `apply`/`rollback`/`remove`/`vendor` hold, so the takeover pre-reverts
141+
/// (lockfiles + the vendored ledger) and the lockfile writes never race
142+
/// them. `acquire`
142143
/// creates a missing `.socket/` and the guard's drop unlinks the lock file
143144
/// and prunes an otherwise-empty `.socket/`, so a run that ends up writing
144145
/// nothing leaves no residue. Contention / IO failures render through the
@@ -299,11 +300,10 @@ async fn installed_stale_positive_evidence(
299300
/// refuses as a write root is still READ here
300301
/// (`verification_only_gem_paths`): bundler installs into it.
301302
/// * Records are found BY UUID (the fetch key, stable across purl
302-
/// spellings): this run's fetched records first, then the redirect
303-
/// ledger's persisted ones — a re-scan whose `/patches/view` fetch failed
304-
/// transiently still re-fires from the ledger instead of silently
305-
/// dropping the warning (`record_fetch_failed` covers the fetch failure
306-
/// itself). Record availability is part of the candidate filter, and the
303+
/// spellings) among this run's fetched records; v5 keeps no hosted
304+
/// ledger to fall back on, so a uuid whose `/patches/view` fetch failed
305+
/// is not judged (`record_fetch_failed` surfaces that failure). Record
306+
/// availability is part of the candidate filter, and the
307307
/// probe returns before any crawler work (or `gem env` subprocess spawn)
308308
/// when no judgment is possible.
309309
/// * PATCHED means [`verify_patch_record`] `Ok` — the one shared oracle
@@ -326,8 +326,7 @@ async fn gem_stale_install_warnings(
326326
global: bool,
327327
global_prefix: Option<std::path::PathBuf>,
328328
confirmed: &[(String, String)],
329-
// This run's fetched records MERGED with the ledger's persisted ones
330-
// (the caller hands the post-merge ledger map).
329+
// This run's fetched records, by uuid.
331330
records: &std::collections::BTreeMap<String, socket_patch_core::manifest::schema::PatchRecord>,
332331
gem_artifact_shas: &std::collections::BTreeMap<(String, String), String>,
333332
) -> StaleInstallOutcome {
@@ -629,20 +628,20 @@ pub(super) async fn run_redirect(
629628
/// ([`socket_patch_core::hosted::engine`], over a
630629
/// [`ProjectView::Disk`](socket_patch_core::vendor::lock_inventory::ProjectView));
631630
/// what stays here is what needs the host: reference grants and the other
632-
/// network fetches, the apply lock (wet runs with a grant), the redirect
633-
/// ledger load, the vendored→hosted takeover pre-revert (symlink-checked
634-
/// first), the `pipenv --version` probe, the symlink guard, the ledger
635-
/// merge-then-persist and the file writes, the gem / Python / vlt
631+
/// network fetches, the apply lock (wet runs with a grant), the
632+
/// vendored→hosted takeover pre-revert (symlink-checked first), the
633+
/// `pipenv --version` probe, the symlink guard, the file writes, the gem /
634+
/// Python / vlt
636635
/// stale-install probes, and the optional VEX. Shared VERBATIM by `scan
637636
/// --mode hosted` (its `--json` arm through the `run_redirect` wrapper, its
638637
/// human arm through [`boxed_run_redirect_selected`] in `scan/mod.rs`; both
639638
/// select via `discover_selected`, with no prompt) and by `get --mode
640639
/// hosted` (which pins the advisory-resolved uuid), so all produce
641-
/// identical on-disk results for the same selection. The redirect ledger
642-
/// is loaded HERE, under the apply lock whenever this run holds one (never
643-
/// handed in pre-loaded: a copy read before the lock could merge over a
644-
/// concurrent writer's edits); a dry run or a zero-grant run reads it
645-
/// strictly but writes nothing, quarantine included.
640+
/// identical on-disk results for the same selection. v5 hosted mode keeps
641+
/// no ledger (the lockfiles are the only record); the VENDORED ledger the
642+
/// takeover needs is loaded HERE, under the apply lock whenever this run
643+
/// holds one (never handed in pre-loaded: a copy read before the lock could
644+
/// be saved over a concurrent writer's edits).
646645
///
647646
/// `scan_result` must be `Some` exactly when `common.json` is set (the
648647
/// human/JSON split keys on `common.json`; a `--json` caller passing `None`
@@ -1091,10 +1090,10 @@ pub(crate) async fn run_redirect_selected(
10911090
std::collections::BTreeMap::new();
10921091
let mut record_warnings: Vec<socket_patch_core::patch::redirect::RewriteWarning> = Vec::new();
10931092

1094-
// SYMLINK GUARD (see `engine::guard`) — before the ledger and before any
1095-
// write, dry runs included, so a dry run predicts the refusal. The
1096-
// revert side (replay.rs) already refuses linked files, so the write
1097-
// side must too.
1093+
// SYMLINK GUARD (see `engine::guard`) — before any write, dry runs
1094+
// included, so a dry run predicts the refusal. The revert side (the
1095+
// hosted → upstream restore's staged flush) already refuses linked
1096+
// files, so the write side must too.
10981097
if let Some(refusal) = engine::guard(&view, &done, &candidates) {
10991098
return refuse(common, scan_result.take(), &refusal);
11001099
}
@@ -1176,8 +1175,8 @@ pub(crate) async fn run_redirect_selected(
11761175
// the writes so the warning describes the project as this run leaves it.
11771176
// Idempotent re-scans re-confirm and re-probe, so the warning keeps
11781177
// firing until the stale materialization is actually gone. Skipped
1179-
// EXPLICITLY on --dry-run: the probe's ledger-record fallback would
1180-
// otherwise judge state the run did not (re)create.
1178+
// EXPLICITLY on --dry-run: nothing was written, so the probe would
1179+
// judge state the run did not (re)create.
11811180
let gem_stale: StaleInstallOutcome = if common.dry_run {
11821181
StaleInstallOutcome::default()
11831182
} else {
@@ -1330,7 +1329,7 @@ pub(crate) async fn run_redirect_selected(
13301329
// Stale-flagged purls are EXCLUDED from assume_applied: the same-run
13311330
// envelope carries a redirect_gem_stale_install warning proving the
13321331
// installed materialization unpatched, so attesting that purl from
1333-
// the ledger would contradict the run's own warning. Excluded purls
1332+
// this run's records would contradict the run's own warning. Excluded purls
13341333
// fall back to `vex`'s normal installed-tree verification.
13351334
// A confirmed uuid whose bundled instance the rewriter had to skip
13361335
// (#469) leaves that copy unpatched, so it too is verified, never
@@ -3388,8 +3387,8 @@ mod tests {
33883387

33893388
/// Probe invocation with the default surface (project-local discovery,
33903389
/// no artifact shas) — tests override the knobs they exercise.
3391-
/// `records` is the merged map production hands over (this run's
3392-
/// fetched records plus the ledger's persisted ones).
3390+
/// `records` is the map production hands over: this run's fetched
3391+
/// records, by uuid.
33933392
async fn probe(
33943393
cwd: &std::path::Path,
33953394
confirmed: &[(String, String)],
@@ -3718,25 +3717,22 @@ mod tests {
37183717
);
37193718
}
37203719

3721-
/// RE-FIRE guarantee: when this run's record fetch failed (no fresh
3722-
/// records), the merged map the caller hands over still carries the
3723-
/// redirect ledger's PERSISTED record under whatever purl key the
3724-
/// ledger used — and the probe's uuid lookup judges from it, so a
3725-
/// transient /patches/view failure cannot silently retire the warning
3726-
/// while the stale materialization is still there.
3720+
/// The probe links a record to a confirmed purl by uuid alone: a
3721+
/// record keyed under the API's qualified purl spelling (not the
3722+
/// confirmed purl) must still judge the stale materialization.
37273723
#[tokio::test]
3728-
async fn gem_stale_probe_judges_from_persisted_ledger_records() {
3724+
async fn gem_stale_probe_matches_records_by_uuid_not_purl_key() {
37293725
let stale = tempfile::tempdir().unwrap();
37303726
materialize_gem(stale.path(), GEM_UPSTREAM);
3731-
// Persisted under the API's qualified spelling, not the confirmed
3727+
// Keyed under the API's qualified spelling, not the confirmed
37323728
// purl: only the uuid links them.
3733-
let mut ledger_only = std::collections::BTreeMap::new();
3734-
ledger_only.insert(format!("{GEM_PURL}?platform=ruby"), gem_record());
3735-
let out = probe(stale.path(), &one_confirmed(), &ledger_only).await;
3729+
let mut qualified = std::collections::BTreeMap::new();
3730+
qualified.insert(format!("{GEM_PURL}?platform=ruby"), gem_record());
3731+
let out = probe(stale.path(), &one_confirmed(), &qualified).await;
37363732
assert_eq!(
37373733
out.warnings.len(),
37383734
1,
3739-
"the ledger records must keep the warning firing across flaky fetches"
3735+
"a record keyed under another purl spelling must still match by uuid"
37403736
);
37413737
}
37423738

‎crates/socket-patch-cli/src/commands/scan/policy.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,10 @@ pub(crate) fn load_invocation_policy(args: &ScanArgs) -> Result<InvocationPolicy
6969

7070
/// The marker files of a disk project root: the same lock markers the
7171
/// in-memory engine detects roots by, plus the maven/nuget markers disk
72-
/// scans support. Manifests are not markers (so both engines agree).
72+
/// scans support. When a directory has none of those, its manifests
73+
/// ([`MANIFEST_MARKERS`] and the JVM build files) are its markers instead;
74+
/// the in-memory engine has no such fallback (manifests alone never make
75+
/// a root there), so for a lockless directory the two engines disagree.
7376
pub(crate) fn dir_markers(dir: &Path) -> Vec<String> {
7477
let mut markers: Vec<String> = std::fs::read_dir(dir)
7578
.map(|entries| {

‎crates/socket-patch-core/src/hosted/engine.rs‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
//! 1. [`build_candidates`] — reference grants → rewriter overrides.
99
//! 2. [`bun_lockb_symlinked`] — the binary-lock symlink refusal.
1010
//! 3. vlt artifact preflight ([`super::vlt`]) + [`withhold_everywhere`].
11-
//! 4. (caller) the apply lock, the ledger, the vendored→hosted takeover.
11+
//! 4. (caller) the apply lock and the vendored→hosted takeover.
1212
//! 5. [`read_candidate_files`] → [`wheel_targets`] → (caller) wheel metadata,
1313
//! and [`yarn_berry_manifest_targets`] → (caller) served npm manifests.
1414
//! 6. [`rewrite`] — the rewriters, the pnpm `trustLockfile` and npm
@@ -17,8 +17,7 @@
1717
//!
1818
//! Nothing here writes, spawns, reads the environment or touches the
1919
//! network: every host effect (locking, probes, record fetches, the commit
20-
//! of the rewritten files, the redirect ledger in [`super::ledger`]) stays
21-
//! with the caller.
20+
//! of the rewritten files) stays with the caller.
2221
2322
use std::collections::{BTreeMap, BTreeSet, HashMap};
2423

@@ -1238,14 +1237,13 @@ pub async fn rewrite(
12381237
);
12391238
if let Some((text, edit)) = trust_config_write {
12401239
rewrite.files.insert(PNPM_WORKSPACE_REL.to_string(), text);
1241-
// Appended last: `--revert` walks edits in reverse, so the trust key
1242-
// is unwound before the lock originals are restored.
1240+
// Appended last, after the lock edits it serves. v5 keeps no hosted
1241+
// ledger, so nothing replays these edits; the order is write order.
12431242
rewrite.edits.push(edit);
12441243
}
12451244
if let Some((text, edit)) = npmrc_config_write {
12461245
rewrite.files.insert(NPMRC_REL.to_string(), text);
1247-
// Appended after the lock edits for the same reason: a whole-ledger
1248-
// replay unwinds the setting before the lock originals it served.
1246+
// Appended after the lock edits, like the pnpm trust key above.
12491247
rewrite.edits.push(edit);
12501248
}
12511249
let rewritten: Vec<String> = rewrite
@@ -1906,8 +1904,8 @@ fn file_ecosystem(rel: &str) -> Option<&'static str> {
19061904
.then_some("pypi")
19071905
}
19081906

1909-
/// SYMLINK GUARD — fail-closed, whole rewrite, before the ledger and before
1910-
/// any write (hosted rewrites are transactional). The writer stages next to
1907+
/// SYMLINK GUARD — fail-closed, whole rewrite, before any write (hosted
1908+
/// rewrites are transactional). The writer stages next to
19111909
/// the path and renames over it, which REPLACES a symbolic link with a
19121910
/// detached regular copy: the link target goes stale and a revert restores
19131911
/// bytes but never the link. Applies to every ecosystem's files and to dry

‎crates/socket-patch-core/src/patch/redirect/staged.rs‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,12 @@
1-
//! Staged, fail-closed file I/O shared by the hosted-redirect reverts — the
2-
//! per-purl takeover ([`super::takeover`]) and the whole-ledger replay
3-
//! ([`super::replay`]).
1+
//! Staged, fail-closed file I/O for the hosted → upstream restore
2+
//! ([`super::upstream`]).
43
//!
5-
//! Both reverts resolve every inverse against a STAGED view of the project
6-
//! and let nothing reach disk until all of them have resolved, so a drift
7-
//! refusal leaves the project byte-identical. This module is that staging
8-
//! layer: FIFO-safe reads of untrusted project files, the staged view, and
9-
//! one flush with the same guards on both sides (a symlink or FIFO squatting
10-
//! a path refuses; every write is atomic and keeps the file's mode).
4+
//! The restore resolves every pin against a STAGED view of the project and
5+
//! lets nothing reach disk until all of them have resolved, so a refusal
6+
//! leaves the project byte-identical. This module is that staging layer:
7+
//! FIFO-safe reads of untrusted project files, the staged view, and one
8+
//! flush with the same guards on both sides (a symlink or FIFO squatting a
9+
//! path refuses; every write is atomic and keeps the file's mode).
1110
1211
use std::collections::BTreeMap;
1312
use std::path::Path;

‎crates/socket-patch-core/src/update/release.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@
1313
//! for a fallback that only fires when the redirect shape drifts.
1414
//!
1515
//! All fetch sizes are capped and every request carries an explicit
16-
//! timeout: a hung self-update is strictly worse than a hung scan, so this
17-
//! module does not inherit the API client's no-timeout posture.
16+
//! whole-request deadline ([`UpdateTimeouts`]), unlike the API client's
17+
//! connect + per-read bounds (`api::retry::ApiTimeouts`): a hung
18+
//! self-update is strictly worse than a hung scan.
1819
1920
use std::time::Duration;
2021

0 commit comments

Comments
 (0)