Skip to content

fix(redirect): fail closed on non-npm: berry descriptors + yarn layering regression suite - #131

Merged
Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
fix/redirect-berry-builtin-patch-gate
Jul 24, 2026
Merged

fix(redirect): fail closed on non-npm: berry descriptors + yarn layering regression suite#131
Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
fix/redirect-berry-builtin-patch-gate

Conversation

@mikolalysenko

Copy link
Copy Markdown
Collaborator

The bug (found by adversarial review of new tests, empirically confirmed)

rewrite_yarn_berry (redirect/mod.rs) matched lock blocks by descriptor name only. Hosted-redirecting a package that yarn berry builtin-patches (fsevents, resolve, typescript) therefore emitted TWO edits for the same package and spliced an npm: resolution + replaced checksum under the patch:-protocol key — a corrupted key/resolution pairing with no warning. That's the same error family as the 2026-07 strapi incident that motivated this work (berry's builtin patch: entries for resolve/fsevents).

The fix

Non-npm: descriptor ranges (patch:/workspace:/portal:/link:) are now skipped byte-identically with a redirect_yarn_berry_unsupported_protocol warning — mirroring the vendor backend's existing fail-closed gate in vendor/yarn_berry_lock.rs. The unwrap() in the gate is guarded by the pre-existing all-descriptors-parse check.

Regression suite: vendor_yarn_layering_e2e.rs (9 tests, each RED-verified)

On a strapi-shaped yarn v1 fixture (multi-version ansi-regex + fsevents@1.2.13/2.3.2 + resolve@1.1.7/1.20.0) and a yarn-4 berry fixture with real builtin patch: entries (128-hex checksums):

  • Byte-surgical vendored wiring — untouched sibling and builtin-patch-target blocks stay byte-identical; zero patch: substrings introduced; rewritten file still parseable by the crate's own inventory.
  • Integrity chain — the rewritten block's #sha1 fragment, sha512 SRI line, and ledger sha256/size are all recomputed in-test from the actual vendored tarball bytes.
  • Vendored → hosted layering (the incident's exact flow) — the hosted redirect records the vendored block as original (reversible by splice), output is byte-surgical, second run is a no-op.
  • Revert-after-hosted-overlay pinned — drift-skipped with vendor_lock_entry_drifted warnings and lossy (blob dir deleted, lockfile stays hosted). Pinned as current behavior with a comment; follow-up candidate.
  • Berry builtin patch: coexistence — builtin entries survive vendored wiring and hosted redirect byte-identically, including the previously-corrupting case: redirecting the builtin-patched package itself now skips its patch: entry with the new warning (RED-verified against the pre-gate corruption).

cargo fmt --check clean, clippy zero warnings, 9/9 green on the branch in isolation.

Sibling of #129 (yarn 1 dev-flow e2e); the hidden --mode aliases from the same session are split into their own PR per review request.

🤖 Generated with Claude Code

…ing regression suite

rewrite_yarn_berry matched lock blocks by descriptor NAME only, so a
hosted redirect of a package that yarn berry builtin-patches (fsevents,
resolve, typescript) spliced an `npm:` resolution + checksum under the
`patch:`-protocol key — a corrupted key/resolution pairing in exactly
the 2026-07 strapi incident's error family, emitted with no warning.
Now non-`npm:` descriptor ranges (patch:/workspace:/portal:/link:) are
skipped byte-identically with a redirect_yarn_berry_unsupported_protocol
warning, mirroring the vendor backend's fail-closed gate
(vendor/yarn_berry_lock.rs).

New vendor::yarn_layering_tests (in-crate #[cfg(test)] module, since the
core-dedups cleanup privatized the vendor wiring internals it plumbs;
inventory_npm_lock widened to pub(crate) for the same reason) pins the
incident's flows on a strapi-shaped yarn v1 fixture (multi-version
ansi-regex + fsevents + resolve) and a yarn-4 berry fixture with real
builtin patch: entries — 9 tests, each RED-verified:
- vendored wiring is byte-surgical: untouched sibling/builtin-target
  blocks byte-identical, zero `patch:` strings introduced, file still
  parseable by the crate's own inventory
- integrity chain: #sha1 fragment, sha512 SRI, and ledger sha256/size
  all recomputed from the on-disk vendored tarball
- hosted redirect layered over vendored wiring records the vendored
  block as `original` (reversible), is byte-surgical, and re-runs are
  no-ops
- vendor revert after a hosted overlay: drift-skipped and lossy
  (deletes the blob dir, lockfile stays hosted) — pinned as current
  behavior
- berry builtin patch: entries survive vendored wiring AND hosted
  redirect byte-identically; redirecting the builtin-patched package
  itself now skips its patch: entry with the new warning

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mikolalysenko
Mikola Lysenko (mikolalysenko) force-pushed the fix/redirect-berry-builtin-patch-gate branch from bb4ab34 to 2cadffb Compare July 24, 2026 15:51
@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit 86a6ffc into main Jul 24, 2026
54 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the fix/redirect-berry-builtin-patch-gate branch July 24, 2026 16:29
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Aug 19, 2026
…ck restore (#205)

* test(vendor): RED — drift-skipped revert must keep artifacts + ledger (residual #131)

Flip the deliberate #131 pinning test
classic_vendor_revert_after_hosted_overlay_is_drift_skipped_and_lossy into
the fixed contract (..._keeps_blob): when every wiring record is
drift-skipped ("left alone"), the blob dir must SURVIVE the revert and the
keep must be surfaced (vendor_artifact_kept) — plus a recovery leg proving
a second revert completes once the drift is undone.

Same contract asserted across the npm-family twins' existing drift tests
(npm_lock / pnpm_lock / yarn_berry_lock / bun_lock / yarn_classic_lock:
revert_leaves_drifted_* now require the artifact dir kept + the keep
warning), two new CLI orchestration tests in in_process_vendor.rs
(run_revert and reconcile must retain the state.json entry, record a
COUNTED Skipped vendor_revert_kept — summary.skipped was 0 even when the
skip event was emitted — and never a Removed), and a new yarn-classic e2e
capstone leg (vendored -> hosted overlay -> vendor --revert keeps the
tarball, the kept tarball still installs after the drift is undone, and
the follow-up revert completes to pristine).

RED: 6 core + 2 CLI + 1 e2e tests fail on current main behavior
(remove_tree runs unconditionally after the restore loop; success prunes
the ledger entry).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(vendor): keep artifacts and ledger when revert drift-skips the lock restore

Residual #131 (strapi campaign, adversarially re-confirmed on main): when
vendor --revert found every recorded lock entry re-resolved (a hosted
patch.socket.dev overlay, a registry re-resolve — anything failing the
uuid-dir ownership gate), revert_recorded_block correctly left the lock
alone with vendor_lock_entry_drifted warnings, but the backend still ran
remove_tree(.socket/vendor/npm/<uuid>/) unconditionally after the restore
loop, and run_revert/reconcile_dropped treated the warning-only success as
"removed": PatchAction::Removed + state.entries.remove + save_state. End
state: the lock still pointed at the drifted target while the artifacts,
the ledger entry holding the only pre-vendor originals, and the tarball
the redirect ledger's recorded `original` fragments reference were all
destroyed — a silent-success data loss with a delayed broken-install
hazard.

Fix, per the orphan sweep's invariant (never delete what something still
references):

- RevertOutcome grows `kept_artifact` plus drift_skipped()/keep_artifact()
  helpers (vendor/mod.rs). keep_artifact() pushes an honest
  `vendor_artifact_kept` warning naming the kept dir and the remediation.
- All five npm-family backends with the unconditional remove shape
  (yarn_classic_lock, npm_lock, pnpm_lock, yarn_berry_lock, bun_lock) now
  skip the remove_tree and mark the keep whenever any wiring record was
  drift-skipped.
- run_revert and reconcile_dropped (cli vendor.rs) honor the signal: the
  state.json entry is retained and the outcome is a COUNTED Skipped event
  (`vendor_revert_kept`) recorded via env.record — fixing the sub-bug
  where summary.skipped stayed 0 while skip events were emitted — never a
  Removed; counts stay uninflated (#166 precedent: advisory warnings keep
  bypassing the counters). Human output gains a "Kept N drifted
  package(s)" line.
- Other RevertOutcome construction sites gain `kept_artifact: false`
  (no behavior change outside the npm family).

Once the drift is undone (vendored lock restored — e.g. a future hosted
revert replaying its recorded originals), the next `vendor --revert`
completes exactly as before: lock restored byte-for-byte, artifacts and
ledger pruned.

GREEN: the 6 flipped core tests, the 2 new CLI orchestration tests, and
the new yarn-classic E1 e2e capstone (drift-skip keep -> kept tarball
still installs -> follow-up revert completes to pristine) all pass;
cargo test -p socket-patch-core clean; clippy --workspace --all-features
clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(vendor): drift-skip keep gate converges — already-reverted records are not drift

The keep gate (drift_skipped) keyed on vendor_lock_entry_drifted, but the
npm-family backends emitted that same code for records a PREVIOUS partial
revert had already restored or removed: a restored fragment's registry
value fails the uuid ownership gate ("re-resolved since vendoring"), a
removed berry resolutions entry hits "no longer exists; nothing to
remove", and a rekeyed pnpm/berry block restored under its pre-vendor key
misses the recorded-key lookup ("no longer exists; nothing to restore").
After any partial restore, every later `vendor --revert` therefore
re-classified the converged records as drift and kept the artifact dir +
ledger entry forever — the CLI's own "undo the drift and re-run `vendor
--revert` to finish" remediation could never be satisfied.

Teach every left-alone branch in the five keep-gate backends (npm, bun,
pnpm, yarn classic, yarn berry) to recognize the ALREADY-CONVERGED state
as a silent no-op before warning:

- a live fragment equal to the recorded pre-vendor `original` (restored
  in place: npm/bun entries, classic/berry blocks, pnpm overrides /
  importer deps / snapshot refs, berry takeover resolutions);
- for Added records with no original, the key being absent (berry
  resolutions entry/table, pnpm overrides in package.json /
  pnpm-workspace.yaml / the lock's overrides section);
- for the rekeying restores (berry lock entries, pnpm packages/snapshots
  blocks), the recorded original block being live verbatim under its own
  pre-vendor key when the recorded key no longer matches.

drift_skipped() now fires only on genuine third-party drift the user can
still undo, so the kept artifacts + ledger entry are released the moment
the drift is undone — run 2 completes the revert and prunes everything.

Tests: each backend's drift unit test gains a second-run leg (undo ONLY
the drift; already-reverted records must be silent and the revert must
complete), the CLI keep tests now drift to a value that is neither ours
nor the original, and a new CLI test pins that a lock manually restored
to the pre-vendor original converges (counted Removed, ledger pruned) on
the first revert.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants