Fix hosted yarn classic rescan of v4 pins (#1462)

[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1462

Summary

Hosted yarn classic users upgrading from v4.0.0 (the current npm latest) can have a yarn.lock pin that v4 wrote for a patch whose package.json adds a dependency the lock never locked. yarn installs the patched package without that dependency. On main such a pin passes silently:

• Rescan: with a sha1 grant it reports applied and no warning. With a sha512-only grant it warns, but says the package "is not pinned", which is wrong because the earlier pin is still in the lock.
• VEX: vex and scan --vex attest not_affected after install.

Root cause

• select_classic_artifacts (hosted/engine.rs) skipped every artifact the lock already pins as "<url>#…" when the grant carries a sha1. The served package.json was never read, so the Yarn classic: a patch that adds a dependency to the package's own package.json leaves vendored yarn.lock with a dangling dependency (offline frozen install fails, lock churns), and hosted silently installs without it #591 dependency check in rewrite_yarn_classic_with (patch/redirect/mod.rs) never ran: it only runs when a manifest exists.
• VEX hash-verifies the installed patched files. Those files are patched, so nothing looked at whether the patched package's dependencies are installed.

Fix

1. The served tarball is read for pinned artifacts too. The cheap "pinned already" lookup still runs first. A fully pinned rescan still never reads the lock's blocks (Perf regression: yarn-classic/hosted wall +165% (5096c41a, #1328) #1423), and they are read at most once overall. The cost is that a rescan now fetches one tarball per pinned yarn classic artifact, one after another (the classic fetch loop in scan/hosted.rs is sequential; fetching concurrently would be a follow-up). That is the only way to catch a pin an earlier release wrote, because nothing in the lock records whether the check ran.
2. A stale pin is refused like a fresh one. It gets the redirect_yarn_classic_dep_manifest_unlocked warning, and the detail now says "an earlier run's pin of it is not confirmed … installs the patched package without them" when the lock already pins this artifact. Being refused means it is never confirmed (the refused_yarn_classic_uuids path), so it is listed as a redirect_unconfirmed skip and in-run scan --vex doesn't attest it. Following the contract for unconfirmed rows (Decide: one shape for the --json top-level error (scan and get emit both a string and a {code, message} object) #704), the exit code and status stay unchanged, and the lock is left as found.
3. VEX checks dependencies for patches that change package.json. This applies to hosted npm-family copies only. A non-optional dependencies entry that node can't resolve from the installed copy is omitted with a new dependency_not_installed tag. Resolution follows node's lookup: node_modules/<name> in the copy or in any non-node_modules ancestor, which covers bundled, hoisted, and pnpm/bun-sibling layouts. A copy outside a node_modules tree, an unreadable manifest, or a name that isn't a plain package name never counts as evidence. The check runs only when the patch record touches package.json, so other patches see no change.

CLI_CONTRACT.md documents the new omission tag and the extended warning. The npm/pypi/gem wrappers only dispatch to the binary, so they need no change.

Tests (red on main → green)

Issue item	Test	main	this PR
#1462 sha1 grant: rescan confirmed the v4 pin	scan hosted_yarn_classic_sha1::issue_1462_v4_pin_with_unlocked_dependency_is_not_confirmed_sha1_grant	FAIL (no warning)	ok
#1462 sha512-only grant: pin kept, wrong "not pinned" text	…_sha512_only_grant	FAIL (detail)	ok
#1462 vex attests after install	e2e_vex_lockfile yarn::hosted_patch_adding_an_uninstalled_dependency_is_omitted (classic and berry 2/3/4)	FAIL (attested)	ok
helper: node lookup layouts	core vex::verify::tests::unresolved_npm_dependency_follows_node_lookup	n/a	ok
#1423 read-count guard kept	core classic_artifact_targets_read_the_lock_blocks_at_most_once (now expects pinned artifacts selected, still 0 block reads)	n/a	ok

Commands: cargo test -p socket-patch-cli --all-features --test scan hosted_yarn_classic, cargo test -p socket-patch-cli --all-features --test e2e_vex_lockfile yarn -- --include-ignored, plus cargo fmt --all -- --check (clean), cargo clippy --workspace --all-features -- -D warnings (clean) and cargo test -p socket-patch-core --all-features --lib (6408 pass). Four unrelated core tests fail locally only because this sandbox runs as root, which ignores the read-only permissions they rely on (copy_tree symlinked root, vlt_heal unremovable lock, pypi_poetry / pypi_requirements write failure). The full CLI matrix runs in CI; it didn't fit on the sandbox disk.

/code-review high findings

Fixed in Resolve npm deps from the copy's real path:

• The dependency lookup now starts from the copy's real path, so pnpm, bun isolated and npm linked symlinks into a store find their siblings.
• The file probes run through run_blocking.
• It reuses npm_common::is_safe_npm_name.
• The omission wording no longer claims the lockfile is at fault.

Not changed, with reasons:

• A failed tarball fetch now drops an already-pinned dep. This is intended and fails closed. A rescan that cannot read the served manifest cannot tell a good v5 pin from a v4 one (Hosted yarn classic rescan and VEX accept a pin written by v4.0.0 whose patched package.json adds a dependency the lock doesn't have, so yarn installs the patched package without it #1462), so it no longer confirms the pin. The lock is left as it was.
• Every pinned artifact is fetched on every rescan (Perf regression: yarn-classic/hosted wall +165% (5096c41a, #1328) #1423 cost). Nothing in the lock records that the check ran (v5 keeps no hosted ledger), so a per-pin marker would need a new persisted file. The fetches run one after another, and the lock's blocks are still never read on a fully pinned rescan. Concurrent fetches or a ledger-based skip would be a follow-up. A ledger-based skip would be a follow-up.
• The check covers pre-existing deps, not only added ones. The record keeps only hashes, so the pristine manifest is not available offline. An installed copy missing a declared non-optional dependency is broken whatever the cause, so withholding the attestation is the safe answer.
• A hosted URL in a comment or a file: block selects a fetch. This needs a hand-edited lock. The cost is one fetch, and the rewriter still skips non-registry blocks.
• Earlier pin under a rotated URL token gets the "not pinned" wording. This affects the warning text only: the dep is refused and unconfirmed either way.
• Per-refused-dep block scan. This runs only on the refusal path, which is rare and needs a network fetch anyway.
• The missing dependency's name is not in the omission. The omission is a stable tag, and the name is easy to see in the patched package.json.

Not covered

Lock-only vex (nothing installed) still attests a stale v4 pin from its wiring. With no installed copy and no network fetch there is no local evidence of the served manifest. Whether hosted VEX should need installed evidence by default is the open #1099 decision.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VDPgtA8XT3RDR96zCibCmo

---

Note

Medium Risk
Changes hosted scan confirmation and VEX attestation for npm/yarn classic; rescans may fetch more tarballs and withhold attestations until dependencies are installed, but locks are left unchanged and behavior fails closed on manifest mismatch.

Overview
Fixes #1462: hosted yarn classic pins from older releases could stay “confirmed” even when the served patch adds a package.json dependency that yarn.lock never locked—yarn still installs the patched files, but not that dependency.

Hosted scan/rescan now fetches and inspects the served tarball for already-pinned classic artifacts (not only fresh pins), so the existing unlocked-dependency check runs on every run. Stale pins get redirect_yarn_classic_dep_manifest_unlocked with wording that the earlier pin is not confirmed, land in refused_yarn_classic_uuids, and are treated like other unconfirmed hosted rows (lock unchanged, exit policy unchanged per #704).

VEX adds a hosted npm-family guard after file verification: if the patch touches package.json, any non-optional declared dependency that Node cannot resolve from the installed copy (hoisted, bundled, or store-sibling layouts via real path) omits attestation with new tag dependency_not_installed. Optional deps and copies outside node_modules are excluded.

CLI_CONTRACT.md, CLI omission messaging, and e2e/scan/core tests document and lock in the behavior.

^{Reviewed by Cursor Bugbot for commit 5fffa69. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Ready for review (burn-down agent)

• Head: fdde961b06784ba56fef4625555d5ab65cadd79a
• CI: 35/35 green, 19 path-skipped; no main-wide failures.
• Bugbot: reviewed this head, check passed. Its one earlier finding (unreadable manifest in vex/verify.rs) was fixed in fdde961 and the thread is resolved.
• Mergeable, CHANGELOG.md untouched.
• Reviewer: note the rescan now fetches one tarball per already-pinned yarn classic artifact (the cost of catching v4-era pins), and the new dependency_not_installed VEX omission tag in CLI_CONTRACT.md.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: A hosted yarn classic rescan used to skip every artifact yarn.lock already pinned, so it never read the served package.json. A v4.0.0 pin for a patch that adds a dependency the lock never locked therefore passed as applied. The rescan now fetches the served tarball for pinned artifacts too, and a stale pin like that gets the redirect_yarn_classic_dep_manifest_unlocked warning, stays unconfirmed, and the lock is left as found. Separately, VEX now omits a hosted npm-family copy, with the new dependency_not_installed tag, when its patch touches package.json and node can't resolve one of its non-optional dependencies. Fixes #1462.

Risk: low to medium. Every change fails closed: rows are left unconfirmed or omitted, never wrongly attested. The medium part is operational. Each scan --mode hosted now downloads every pinned yarn classic tarball, and a failed fetch turns a pin that used to confirm into an npm_tarball_unavailable skip. The PR description says this is intended.

Look here:

• engine.rs:1294-1320: the new selection, pinned OR registry copy.
• scan/hosted.rs:1221-1241:''`` the fetch loop, where a failure drops an already-pinned patch.
• redirect/mod.rs:4097-4125:'''``` how a stale pin is refused, with the "earlier run's pin" wording.
• vex/verify.rs:482-549: unresolved_npm_dependency / node_resolves. Lookup follows node's rules from the copy's real path, and names are checked with is_safe_npm_name before any join.
• CLI_CONTRACT.md:1445: the new omission tag.

Verified:

• Read the full diff.
• Ran cargo test -p socket-patch-core --all-features --lib -- vex::verify classic_artifact_targets (41 pass).
• The two issue_1462_* scan tests would fail on main.
• Path traversal (../../escape is tested), FIFO manifests, pnpm/bun symlinked store layouts and Windows \\?\ paths are all handled.
• No debug leftovers and no scope creep. CHANGELOG.md untouched.
• CI on fdde961: ci-ok and clippy green. Cursor Bugbot passed. No unresolved threads.

Changes I made: none.

Open questions (non-blocking):

1. The description says the pinned-tarball fetches run "concurrently, from Fix yarn classic hosted scan lock rescans (#1423) #1426". The classic fetch loop at scan/hosted.rs:1221 is actually sequential (for … await). A lock with many hosted classic pins pays N sequential downloads on every rescan. A follow-up could fetch them concurrently.
2. The CLI_CONTRACT row says the dependency is missing "because the lockfile never locked it". The check covers any hosted npm-family copy with any unresolvable non-optional dependency, so "for example" would be more accurate.

Update for 5fffa69: the only commit since this brief is a one-line docs change to CLI_CONTRACT.md (dependency_not_installed row now says "for example because…" and "above its real path"). That settles open question 2. Question 1 is now a follow-up noted in the PR description. CI on 5fffa69: ci-ok and clippy green.

Auto-merge (squash) is armed, so approving sends it straight to the merge queue.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

[agent] Both open questions in the review brief are right.

1. The classic fetch loop is sequential, as the brief says. I've corrected the PR description; making it concurrent is left as a follow-up.
2. I've widened the CLI_CONTRACT dependency_not_installed row in 5fffa69 to say "for example because the lockfile never locked it". The change is docs only.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

bugbot run

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 5fffa69. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Still Ready for review at 5fffa6975b (burn-down agent re-check after the docs-only CLI_CONTRACT.md commit): CI 34/34 green + 19 path-skipped, Bugbot reviewed 5fffa69 with no findings, mergeable, CHANGELOG.md untouched.

---

Generated by Claude Code