Skip to content

Fix VEX attesting beside an unpatched same-lock copy (#935, #938, #939) - #940

Merged
Mikola Lysenko (mikolalysenko) merged 8 commits into
mainfrom
agent/fix-same-lock-unpatched-copy-vex
Oct 7, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 8 commits into
mainfrom
agent/fix-same-lock-unpatched-copy-vex

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

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

Fixes #938
Refs #935
Refs #939

Summary

A lockfile-only vex attested a package as not_affected even though the same lock also installed an unpatched copy of that exact name@version:

Root cause (shared)

VEX discovery had no shared same-lock rule. contest_across_locks (vex/discover/mod.rs) skips evidence from the ref's own lock (e.file != r.source_file), so each extractor wrote its own same-lock check. Only npm (push_uncontested unwired) and yarn classic git copies had one.

Change

Why #935 and #939 are Refs, not Fixes

Both issues also ask the hosted / vendored scans to warn about the unreached copy. That is a separate gap in four rewriters (pnpm and berry, hosted and vendored), not this discovery boundary. This PR fixes the false VEX attestation for both. The scan warnings are left as a follow-up on those issues.

Test evidence

Regression tests, red with contest_within_locks disabled and green with it:

Issue Test
#935 vex::discover::npm::tests::issue_935_same_lock_file_copy_contests_the_pnpm_ref: v9 dir, v9 tgz, legacy dir, plus controls (wiring alone; dir of another version)
#938 vex::discover::yarn::tests::issue_938_classic_registry_block_of_the_same_version_contests_the_ref: plus control (registry block of another version)
#939 vex::discover::yarn::tests::issue_939_berry_other_name_copy_contests_the_ref: file: tgz, file: dir, registry url, plus controls (wiring alone; file: copies holding another package)

The existing classic_git_pattern_copies_are_never_attested also went red with the shared pass disabled, which confirms git copies now go through it.

Local runs:

  • cargo test -p socket-patch-core --all-features --lib: 5248 passed. 4 permission-based tests fail only because this sandbox runs as root (copy_tree symlinked root, vlt_heal unremovable lock, poetry / requirements write failure). They pass in CI on main's head. The 5th failure was the digest guard, fixed by the Route Gradle digests through utils::digest #878 port.
  • cargo test -p socket-patch-cli --all-features --lib --test e2e_vex_lockfile --test e2e_vex_redirect --test e2e_vex_vendor --test e2e_vex --test covgap_commands_vex: all green (847 + 12 + 19 + 311 + 31 + 28).
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • VEX discovery golden (redirect-npm.json): regenerated. The only change is an additive unpatched_copies section on yarn classic fixtures. No ref or diagnostic changed.
  • cargo fmt: touched files are formatted. main itself isn't cargo fmt --check clean (about 120 files), and CI doesn't run fmt.
  • Scan bench: the first push regressed yarn-classic/hosted / rescan by 16–18% because of a quadratic per-insert dedup. 980b7b6 removes it. A local socket-patch-bench compare --filter yarn-classic against main then shows +1.4% [-10.4, +8.1] and -3.0% [-8.1, +3.3], both ≈.

The npm / PyPI / gem wrappers don't need changes: this is Rust-only discovery logic.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RpNijzXr9S3AZY6xHUDwVH


Note

Medium Risk
Changes VEX discovery and attestation eligibility for npm-family locks when duplicate same-version install paths exist; behavior is fail-closed (drops refs, emits diagnostics) but affects security-relevant VEX output.

Overview
Stops false VEX not_affected claims when a lockfile both wires a package to a Socket patch and still installs an unpatched copy of the same name@version in that same lock—something cross-lock contest_across_locks never saw because it ignores evidence from the ref’s own file.

Discovery now has a shared unpatched_copy / contest_within_locks path: extractors record competing lock entries (with key + how they install), those copies also count as resolved_elsewhere, and matching refs are dropped with patched_ref_unattributable diagnostics that name the copy. The pass runs after all extractors and before cross-lock contesting.

pnpm (#935): file: directory and tarball entries (v9 and legacy keys); package identity from lock fields or the copy’s package.json / tarball.

Yarn classic (#938): non-Socket registry tarball blocks beside a Socket block (replacing a yarn-only post-filter); git and file: directory copies go through the same API.

Yarn berry (#939): file: / URL locators keyed under another dependency name; real package name from tarball, directory manifest, or registry URL path.

Golden discovery output gains an unpatched_copies section; regression tests cover the three issues plus controls (wrong version, different package, wiring alone).

Reviewed by Cursor Bugbot for commit acbac79. Configure here.

Assisted-by: Claude Code:claude-opus-5-5
A lockfile-only `vex` attested a package as not_affected while the same
lock also installed an unpatched copy of that exact name@version:

- pnpm: a `file:` directory or tarball copy (#935)
- yarn classic: a registry block left beside the Socket block, e.g.
  after `yarn add -W left-pad --exact` (#938)
- yarn berry: a `file:` / url copy locked under another dependency
  name, which the `resolutions` pin never reaches (#939)

The cross-lock contest only weighs OTHER locks, and each extractor
wrote its own same-lock rule (npm and yarn classic git only). Discovery
now has one shared same-lock rule: extractors record an unpatched copy
and every ref of the same name@version in that lock is dropped with a
patched_ref_unattributable diagnostic naming the copy. Yarn classic's
git-copy filter moves onto it. The berry and pnpm extractors read the
copy's real package from its package.json (directory or tarball) or
from the registry tarball url.

Assisted-by: Claude Code:claude-opus-5-5
main has failed socket-patch-core's lib tests since Gradle support
(#646) and the digest helpers (#865) both landed. The guard test
production_digests_go_through_the_helpers flags three files #646 added
that still hash inline: crawlers/gradle_cache.rs, patch/jvm_jar.rs and
patch/sidecars/maven.rs. That breaks test, test-release and coverage on
every open PR.

Each inline sha1/sha256 call now goes through sha1_hex_of or
sha256_hex_of, which compute the same lowercase hex. Behaviour is
unchanged.

Assisted-by: Claude Code:claude-opus-5-5
(cherry picked from commit 659ac2c)
Every non-Socket yarn classic block is now recorded as a possible
unpatched copy, and each record scanned the whole list for a duplicate
first. On a 3000-package lock that made hosted scans and rescans about
17% slower in the scan benchmark. The list is already sorted and
deduplicated once when discovery finishes, so the per-record scan goes.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 6, 2026 14:25
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] native (windows-latest, 1.2.0) (Bun patch compatibility, run 37476785866) failed on one cell: 1.2.0 workspace vendored → refusalCodesExact. The other 48 cells passed. I think this is the patch API, not this PR, but I haven't confirmed it yet:

  • the cell took ~15 s against ~1 s for its neighbours, and finished in the same second another cell got API request failed with status 504. That cell passed on the script's own retry.
  • this PR only changes the npm / pnpm / yarn VEX discovery readers. The Bun reader (vex/discover/bun.rs) and the Bun vendored backend are untouched.
  • the same workspace shape passed in hosted mode in this job, on every other Bun version on Windows, and on macOS.

The run is still in progress, so GitHub won't re-run the failed job yet (403 "already running"). I'll re-run it once when the run finishes. If it fails again, I'll pull captures/1.2.0-workspace-vendored/result.json from the job's results artifact and treat it as a real failure.


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

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.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 6, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review at 980b7b6.

  • CI: 517/517 latest check runs green or skipped. The one red job, native (windows-latest, 1.2.0) in Bun patch compatibility, failed at the install step. It passed on a rerun of the same commit. The PR doesn't touch Bun code.
  • Bugbot: reviewed 980b7b6 with no findings, and no review threads are unresolved.
  • Mergeability: 0 commits behind main, no conflicts. It's waiting only on a required human review.
  • For reviewers: the shared same-lock rule in vex/discover/mod.rs and how it's used in npm.rs and yarn.rs, plus the updated redirect-npm.json golden.

Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 7, 2026
Conflicts:
- vex/discover/mod.rs: kept both main's ContestedRef/`contested` and
  this branch's UnpatchedCopy/`unpatched_copies` (structs, fields,
  finalize); `contest_within_locks` still runs before
  `contest_across_locks`, after main's new sbt extractor.
- vex/discover/testing/golden.rs: import and render both `contested`
  and `unpatched_copies`.
- vex/discover/yarn.rs: kept main's ClassicBlockSource match and
  classic_block_purl; main's #921 `file:` directory copies and the git
  copies now go through the shared `Discovery::unpatched_copy` rule
  instead of main's local post-filter loop (same diagnostic: names the
  lock entry and "file: directory").

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

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

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.

@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit c5be5d1 into main Oct 7, 2026
88 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-same-lock-unpatched-copy-vex branch October 7, 2026 15:36
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 7, 2026
- `scan --prune --dry-run` previewed the vendor GC's manifest drop with a
  qualifier-strip relation while the wet pass used PurlKey, so NuGet case,
  PEP 503 and composer padding variants were pruned for real but not in
  the preview. Both now call vendor::unused_vendored_manifest_keys.
- LockfileSupplement.purls is a HashSet<PurlKey> built once, so the
  lockfile-only predicate is one hash lookup instead of re-keying the
  whole set on every miss.
- Ledgers::hosted_vendored_overlap deduplicates by PurlKey, so two
  spellings of one release give one takeover warning.
- get's hosted-claim set (rebased onto #940's new code) is a
  HashSet<PurlKey>; the new gem takeover pin lookup uses PurlKey::same.
- composer_version: pin that the sentinel-free key PurlKey uses never
  lets a rejected spelling collide with an accepted one, and document it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 7, 2026
Merges cleanly; additionally routes the yarn berry url-copy VEX
justification (added on main in #940) through redact_url so a
credentialed tarball URL never reaches the published VEX document.

Co-Authored-By: Claude <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 7, 2026
- `scan --prune --dry-run` previewed the vendor GC's manifest drop with a
  qualifier-strip relation while the wet pass used PurlKey, so NuGet case,
  PEP 503 and composer padding variants were pruned for real but not in
  the preview. Both now call vendor::unused_vendored_manifest_keys.
- LockfileSupplement.purls is a HashSet<PurlKey> built once, so the
  lockfile-only predicate is one hash lookup instead of re-keying the
  whole set on every miss.
- Ledgers::hosted_vendored_overlap deduplicates by PurlKey, so two
  spellings of one release give one takeover warning.
- get's hosted-claim set (rebased onto #940's new code) is a
  HashSet<PurlKey>; the new gem takeover pin lookup uses PurlKey::same.
- composer_version: pin that the sentinel-free key PurlKey uses never
  lets a rejected spelling collide with an accepted one, and document it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run

Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 7, 2026
Resolve conflicts in vex/discover: keep main's same-lock contest
(#940) alongside this branch's unwired-copy unattestation, pnpm bundled
copies and berry registry-locator contest.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 acbac79. Configure here.

Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 7, 2026
After merging main's #940 same-lock unpatched-copy contest into the
#828 shadowing, a pin withheld because a pnpm file: dir/tarball, a yarn
classic registry/file: block, or a berry other-name file:/url copy
installs beside it is shadowed and restored like any other pin. Extend
the CLI contract sentence that only named bundled, git and
hasShrinkwrap-nested copies.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Merged at acbac79 (merge commit c5be5d1). Wrap-up:

  • Conflicts: none. I merged main into the branch after Fix main CI red on stale digest pending-list entries #1016 landed (869c81f). A maintainer then merged main again (acbac79). The PR diff stayed limited to vex/discover/{mod,npm,yarn}.rs, the golden harness and redirect-npm.json.
  • Local checks: clippy on socket-patch-core with -D warnings was clean. All 5594 socket-patch-core lib tests passed on acbac79, including the digest guard.
  • CI on acbac79: the CI, Gradle, PDM, Pipenv, Go, Composer, sbt, npm and pnpm workflows passed. One red job: vlt / native (ubuntu-latest, 1.0.10), cell hosted-two-versions. Its capture shows redirect_vlt_artifact_unverifiable … fetch error error sending request against patch.socket.dev. That's a network flake, and this PR doesn't touch vlt or redirect code. The vendored and agent cells for the same fixture passed. The Bun and vlt runs were still queued when the PR merged.
  • Bugbot: reviewed acbac79 and found no new issues.

Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants