GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix capped scan deferring contested npm pins (#1195) by mikolalysenko · Pull Request #1211 · SocketDev/socket-patch

Conversation

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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

Fixes #1195

Summary

A capped scan (--max-new-patches 0, SOCKET_MAX_NEW_PATCHES=0, or
socket.yml patches.maxNewPatches: 0) deferred a hosted patch that was
already applied, as if it were NEW. This happened once the project gained
a second, unpinned copy of the same name@version: an npm: alias, an
unpinned npm-shrinkwrap.json twin, or a bundled copy. The re-scan that
would rewire that copy never ran, so the remedy vex prints looped
forever and npm ci kept installing the unpatched copy.

Root cause

Since #1058 the scan's recorded view (update detection and the rollout
cap) is built from HostedPin::all(discovery), which returns attributable
pins only. Discovery drops a pin from refs when an unpatched copy of the
same version installs beside it. That is right for attestation, but the
rollout asks whether anything is recorded for the purl, and a pin in a
live lock is still recorded. The same drop happens in several places, so
the fix goes at that shared boundary rather than in the npm extractor
alone:

npm: push_uncontested (same-lock entry, bundled copy, twin lock
resolving elsewhere or missing the entry), the npm 6 legacy mirror and
non-registry entries
Bun: same-lock and bundled copies
vlt: installed bundled copies
the shared Discovery::contest_within_locks (pnpm/yarn file:
copies) and Discovery::contest_across_locks

Change

Discovery::shadowed keeps every ref dropped at those sites. It is
sorted like refs and covered by the discovery goldens. Each such ref
already has a DIAG_REF_UNATTRIBUTABLE diagnostic.
HostedPin::recorded / HostedPin::discover_recorded = attributable
pins plus shadowed ones.
The rollout's recorded view uses them in the disk scan
(scan/mod.rs update manifest and RecordedIndex, plus the
foreign-origin re-pass in scan/hosted.rs) and in the in-memory engine
(hosted/memory).
Attestation, redirect state, takeover and the management commands still
use HostedPin::all and are unchanged. A uuid that a stale file merely
mentions still doesn't count (Decide whether a hosted patch is pinned through lockfile discovery alone #1058's intent): only real lock entries
are shadowed.

No wrapper changes needed (npm/pypi/gem wrappers only dispatch the
binary).

Test evidence

Issue	Test	Red on main	Green with fix
#1195 alias shape	`scan_rollout_e2e::issue_1195_a_capped_rescan_rewires_an_unpinned_alias_copy`	counts `(new 0, deferred 1, upgrade 0, already 0)`	`(0,0,0,1)`, both lock entries pinned
#1195 dual-lock shape	`scan_rollout_e2e::issue_1195_a_capped_rescan_rewires_an_unpinned_twin_lock`	`deferred 1`	ALREADY, shrinkwrap rewired
every drop rule	`vex::discover::npm::tests::issue_1195_withheld_refs_stay_recorded_pins` (alias, bundled, twin registry, twin missing, pnpm `file:`) plus new assertions in the Bun #588 and vlt bundled tests	(new API)	pass

The red tests were committed alone first (1d1d6a9); the fix is 93e1993.

Run locally:

cargo fmt --all -- --check: clean
cargo clippy --workspace --all-features -- -D warnings: clean
cargo test -p socket-patch-cli --all-features --lib --bins: 897 passed
cargo test -p socket-patch-cli --all-features --test scan_rollout_e2e --test hosted_memory_rollout: 17 + 8 passed
cargo test -p socket-patch-core --all-features --test hosted_inventory --test upstream_restore_golden: 9 + 50 passed
cargo test -p socket-patch-core --all-features --lib: 5847 passed, 4 failed. The 4 are copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry, pypi_poetry::wire_write_failure_maps_error… and pypi_requirements::wire_failure_rolls_back…. All four test read-only-permission failure paths and fail only because the sandbox runs as uid 0. They are in files this PR doesn't touch; CI runs them as non-root.
A full cargo test --workspace couldn't link all ~240 test binaries in the sandbox's disk allowance, so CI covers the rest.

🤖 Generated with Claude Code

https://claude.ai/code/session_018DZMq1J4wpPK5Y7gTYsNSM

Note

Medium Risk
Changes how hosted rollout decides NEW vs ALREADY for contested lock wiring; wrong classification could defer fixes or over-apply patches, but scope is discovery/rollout boundaries with strong regression tests.

Overview
Fixes #1195, where a capped hosted scan (--max-new-patches 0) treated an already-pinned package as NEW and deferred it when a second, still-unpatched copy of the same name@version appeared (e.g. an npm: alias or an unpinned twin lock).

Discovery now tracks those refs in a new rewirable bucket (alongside existing shadowed), populated wherever a pin is withheld from attestation but a re-run can rewire the other copy—npm/Bun same-lock contests, twin locks, legacy npm 6 mirror, cross-lock contests, etc. HostedPin::recorded / discover_recorded include rewirable pins; the scan rollout recorded view (update manifest, foreign-origin pin pass, in-memory engine) uses them so capped re-scans classify the patch as ALREADY and rewire the extra copy. Attestation and management commands still use HostedPin::all (attributable pins only).

E2E and unit tests cover alias and dual-lock re-scan behavior plus golden fixture updates for contested npm wiring.

^{Reviewed by Cursor Bugbot for commit 328fb35. Configure here.}

Generated by Claude Code

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Start fix for #1195

bfc7778

Assisted-by: Claude Code:claude-opus-5-5

Mikola Lysenko (mikolalysenko) mentioned this pull request

Oct 9, 2026

Capped hosted scan (--max-new-patches 0) defers an already-pinned npm patch as NEW when a second lock entry of the same version is unpinned, so the vex remedy loops and that copy stays unpatched (regression from #1058) #1195

Closed

Claude (claude) added 2 commits

October 9, 2026 02:29


          Test capped re-scans of contested npm pins

1d1d6a9

Add end-to-end regressions for #1195: a hosted pin whose lock later
gains an unpinned npm: alias copy of the same version, and a pinned
package-lock.json beside an unpinned npm-shrinkwrap.json. Under
--max-new-patches 0 both re-scans currently defer the already-applied
patch as new and leave the second copy unpatched.

Assisted-by: Claude Code:claude-opus-5-5


          Count shadowed hosted pins as already applied

93e1993

A capped scan (--max-new-patches 0, SOCKET_MAX_NEW_PATCHES=0 or
socket.yml maxNewPatches: 0) deferred an already-applied hosted patch
as NEW once the project gained a second, unpinned copy of the same
version: an npm: alias, an unpinned npm-shrinkwrap.json twin, a
bundled copy. Lockfile discovery rightly stops attesting such a pin,
but the rollout read "not attestable" as "nothing recorded", so the
re-scan that would rewire the copy never ran and the remedy vex
prints looped forever (#1195, regression from #1058).

Discovery now keeps every ref it drops over an unpatched copy beside
it in Discovery::shadowed (npm's pair, same-lock, bundled and legacy
mirror rules, Bun and vlt copies, and the shared within-lock and
cross-lock contests). HostedPin::recorded adds those to the
attributable pins, and the disk and in-memory rollouts build their
recorded view from it. Attestation and management commands still use
HostedPin::all and are unchanged.

Fixes #1195

Assisted-by: Claude Code:claude-opus-5-5

Mikola Lysenko (mikolalysenko) marked this pull request as ready for review

October 9, 2026 02:58

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Copy link

Copy Markdown

Collaborator Author

BugBot review

Generated by Claude Code

cursor[bot] reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot reviewed

Oct 9, 2026

View reviewed changes

cursor Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Claude (claude) added 2 commits

October 9, 2026 03:56


          Merge main into agent/fix-rollout-contested-pins

d5e0af4

main (#1008) added Discovery::shadowed for pins withheld over a copy
no rewire can reach, and HostedPin::all now includes them so the
management commands unwind them. Keep that as is and record the pins
withheld over a copy a re-run CAN rewire (an npm: alias, the npm twin
lock, another manager's lock, npm 6's legacy mirror, a Bun registry
entry) in a separate Discovery::rewirable, which only the rollout's
recorded view (HostedPin::recorded) reads.

Assisted-by: Claude Code:claude-opus-5-5


          Restore files this branch only reformatted

328fb35

Running cargo fmt during the merge reformatted 16 files main carries
unformatted. They are outside this fix, so take main's bytes back and
keep the diff to the rollout change.

Assisted-by: Claude Code:claude-opus-5-5

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Copy link

Copy Markdown

Collaborator Author

BugBot review

Generated by Claude Code

cursor[bot] reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot reviewed

Oct 9, 2026

View reviewed changes

cursor Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 328fb35. Configure here.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mikola Lysenko (mikolalysenko) added the agent:needs-human label

Oct 9, 2026

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Copy link

Copy Markdown

Collaborator Author

Needs a maintainer decision. This PR conflicts with main, and the conflict is in behavior, not just text (burn-down agent, head 93e1993b, main at f3c6313).

Both this PR (#1195) and #828 (merged to main) added Discovery::shadowed, the refs withheld from attestation because an unpatched copy of the same name@version installs beside them. They disagree on who sees those refs:

npm hosted pin next to a bundled copy can't be unwound: rollback/remove refuse it, and the vendored takeover skips the restore, so vendor --revert lands back on hosted and allow-remote=all stays #828 on main: HostedPin::all (the "is this patch pinned" answer: forward-rewrite confirmation, HostedInventory, rollback / remove / takeover) now includes shadowed. main also only shadows a non-registry/shrinkwrapped copy when the ref's own location isn't foreign.
This PR: HostedPin::all excludes shadowed ("management commands do not act on it"), and only the new HostedPin::recorded (the rollout's view) includes it. The PR also adds shadow sites main doesn't have: the npm pair, same-lock npm: aliases, and twin-lock contests in contest_within_locks / contest_across_locks.

The text conflicts merge mechanically (one shadowed field, main's validating shadow(), both sides' e2e tests). After that, issue_1195_withheld_refs_stay_recorded_pins fails on assert!(HostedPin::all(&out).is_empty()) for every case, because main now counts those refs as pins.

Question: should twin-lock and alias-contested pins (this PR's new shadow sites) count as pins for rollback / remove / takeover and the forward rewrite's confirmation, the way #828 does for bundled and same-lock copies?

Yes: take main's semantics. recorded becomes the same as all and can be dropped, and the 1195 test's all(...).is_empty() assertions become == recorded.
No: keep a separate attested-only set for those sites, e.g. a second contested_shadowed list that recorded reads and all doesn't.

I left the branch unmerged so nothing half-resolved lands. Reply with yes or no and the next run will do the merge.

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Copy link

Copy Markdown

Collaborator Author

[agent] This is already resolved on the current head, 328fb35 (merge d5e0af4 plus a restore commit), using the No option. Please don't re-merge.

Discovery::shadowed, shadow() and HostedPin::all are byte-for-byte main's npm hosted pin next to a bundled copy can't be unwound: rollback/remove refuse it, and the vendored takeover skips the restore, so vendor --revert lands back on hosted and allow-remote=all stays #828 semantics. Rollback, remove, takeover and the forward-rewrite confirmation see exactly what they see on main.
The sites this PR adds (the npm same-lock entry / npm: alias, the twin lock resolving elsewhere or missing the entry, npm 6's legacy mirror, Bun same-lock registry entries, contest_across_locks) go into a separate Discovery::rewirable. Only HostedPin::recorded (the rollout's recorded view) reads it. These are copies a re-run can rewire, so management commands keep refusing around them as before. Widening npm hosted pin next to a bundled copy can't be unwound: rollback/remove refuse it, and the vendored takeover skips the restore, so vendor --revert lands back on hosted and allow-remote=all stays #828's unwind to them would be a separate behavior change, outside Capped hosted scan (--max-new-patches 0) defers an already-pinned npm patch as NEW when a second lock entry of the same version is unpinned, so the vex remedy loops and that copy stays unpatched (regression from #1058) #1195.
The unit test is now issue_1195_rewirable_refs_stay_recorded_pins. It asserts shadowed and HostedPin::all are empty, and recorded is the pin, for each rewirable case. It passes, along with both Capped hosted scan (--max-new-patches 0) defers an already-pinned npm patch as NEW when a second lock entry of the same version is unpinned, so the vex remedy loops and that copy stays unpatched (regression from #1058) #1195 e2e tests in scan_rollout_e2e (18/18).
Bugbot reviewed 328fb35 with no findings. CI is running.

If a maintainer would rather have those pins managed like #828's, it's a one-line change in HostedPin::all plus test updates, best done as a follow-up.

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) mentioned this pull request

Oct 9, 2026

Capped hosted gem re-scan counts a Gemfile-only (no-CHECKSUMS) pin as NEW, so --max-new-patches N spends its budget on gems it already wired and starves the next one forever (regression from #1058) #1224

Closed

Tanmay Singla (Tanmay182003) approved these changes

Oct 9, 2026

View reviewed changes


          Merge remote-tracking branch 'origin/main' into agent/fix-rollout-con…

982a6f6

…tested-pins

# Conflicts:
#	crates/socket-patch-cli/src/commands/scan/mod.rs
#	crates/socket-patch-core/src/hosted/memory/mod.rs

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Copy link

Copy Markdown

Collaborator Author

Merged main (e03a666d) into this branch at 982a6f67 to clear conflicts with #1228 (#1224) in scan/mod.rs and hosted/memory/mod.rs: kept this PR's HostedPin::recorded view and added main's Gemfile-only gem pins on top. Local: scan_rollout_e2e 18/18, core hosted:: 119/119, vex:: 662/662. agent:needs-human left as is.

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) added this pull request to the merge queue

Oct 9, 2026

Tanmay Singla (Tanmay182003) approved these changes

Oct 9, 2026

View reviewed changes

Merged via the queue into main with commit c2f997b

Oct 9, 2026

467 checks passed

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) deleted the agent/fix-rollout-contested-pins branch

October 9, 2026 13:25

Reviewers

cursor[bot]

cursor[bot] left review comments

Tanmay Singla (Tanmay182003)

Tanmay182003 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

agent:needs-human

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Summary

Root cause

Change

Test evidence

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mikola Lysenko (mikolalysenko) commented Oct 9, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.