Skip to content

Stop attesting NuGet versions the writers refuse - #2007

Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
fix/nuget-discovery-other-version
Open

Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
fix/nuget-discovery-other-version

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Sometimes a governed packages.lock.json locks a Socket-mapped NuGet id at a version other than the patched one: in another target framework, or in another project's governed lock. Both writers refuse to wire that:

  • hosted: rewrite_nuget → redirect_nuget_lock_other_version
  • vendored: nuget_prelude → vendor_nuget_lock_other_version

The exact-id <packageSourceMapping> sends every version of the id to a source that serves only the patched version, so that framework can't restore. Discovery (vex/discover/nuget.rs) still emitted one attested ref per locked version, the unpatched one included. It also keyed versions by raw resolved.trim(), while the writers match through normalize_nuget_version. As a result, 13.0.3 and 13.0.3.0 became two refs, each with its own pin check.

Fix

  • Normalized versions. Lock entries are keyed by normalize_nuget_version(resolved.trim()).
    • The purl keeps the first spelling seen; purls compare through the same normalization (PurlKey).
    • An empty resolved gets patched_ref_invalid. The writers would count it as another version, so it withholds the statement the same way.
    • Only Socket-mapped ids are indexed.
  • The writers' other-version rule. Each ref is checked against the versions locked other than its own. If there are any, the ref stays a ref but is marked with the new UnattestedKind::NugetLockOtherVersion. The VEX plan then omits it with vex_nuget_lock_other_version. The note names the other versions in NuGet precedence order, and the lock that holds the first of them.
    • Vendored: the feed's single <idLower>.<versionNorm>.nupkg names the patched version, and that is the only ref, pinned by the lock entries at it if there are any. It is still the ref when no lock entry is at that version, so the ledger's vendored_claim stays live. Other locked versions show up only in the detail, never as refs that point at nupkgs that don't exist. With no single feed nupkg (no feed, or several nupkgs), the lock's versions are the refs, checked the same way as hosted.
    • Hosted: the url names only the uuid. If two versions are locked, one of them is unpatched, so every ref of that source is withheld. A single locked version that isn't the patch's is already gated by the VEX plan as record_mismatch (see finding 4).

Why an unattested kind rather than a diagnostic plus dropping the ref

Dropping the refs (or moving them to shadowed) makes hosted_claim / vendored_claim return Some(false) for the ledger record of the patched version. Then scan, vendor --check, rollback and remove would treat Socket's own wiring, which is still present, as reverted. Re-running scan / vendor can't clear that either. Unattested is meant for exactly this case.

Review findings (/code-review high)

  1. Confirmed, fixed. When the vendored lock named only a version the feed doesn't serve, the only ref pointed at a missing nupkg, and vendored_claim on the real artifact returned Some(false). The feed's artifact is now the ref, unpinned and unattested. The test asserts vendored_claim == Some(true).
  2. Confirmed, fixed. Vendored refs were emitted for locked versions the feed doesn't serve. Now only the feed's version is a ref, and the others appear in the detail.
  3. Refuted. The suggestion was to apply the hosted rule only when the Socket mapping is exclusive. Both writers refuse another locked version whether or not the mapping is exclusive: the check runs before wiring. Both also comment out any competing exact-id pattern when they wire (set_aside_competing_patterns, Hosted/vendored NuGet mapping isn't exclusive when nuget.config already maps the exact package id to another source, so restore races the Socket feed against nuget.org (NU1403 with a lock, silently unpatched without one) #462). So a non-exclusive mapping is never writer output. With one in place, the patched version's lock contentHash fails a restore from the competing source anyway. This module's non-goals already say so.
  4. Considered, not moved. Moving the hosted rule into the VEX plan would only add coverage for the single-wrong-version case, and the plan already handles that:
    • A hosted ref at 13.0.1 for a patch recorded at 13.0.3 becomes a lockfile-only candidate. Its record, whether local (local_record_by_uuid) or from the API, names another package, so it is gated record_mismatch. Without a record it is gated record_unavailable.
    • The ledger candidate at 13.0.3 is dead, because hosted_claim finds no ref wiring 13.0.3.
    • In the multi-version case, discovery's per-ref check is exact: whichever version the patch is, the other one is locked.
    • Moving it would add a new public Discovery field and a plan pass after record resolution, with no change in behavior. CLI_CONTRACT.md now documents this split.
  5. Confirmed, fixed. " 13.0.3 " next to 13.0.3 used to read as two versions. Values are now trimmed before normalizing, and an empty value is diagnosed.
  6. Confirmed, fixed. The contentHash-conflict diagnostic now names the locks involved and every spelling, e.g. packages.lock.json, src/App/packages.lock.json: Newtonsoft.Json 13.0.3 / 13.0.3.0 ….
  7. Confirmed, fixed. The detail and file now name the lock holding an other version, and versions are listed in NuGet precedence order (13.0.9 before 13.0.10). The detail says "also lock" only when the ref's own version is locked.
  8. Partly fixed. Only Socket-mapped ids are indexed, and their lock paths are cloned once per version. The vendored feed directory is still read even when the lock has an entry, because the feed is the only file that names the patched version. It is one read_dir of a single uuid directory.
  9. Fixed. A single push_ref helper now marks and pushes refs.

Tests (each fails on the code it fixes, checked by splicing the older extractor under the new tests)

  • hosted_id_locked_at_two_versions_is_unattested covers:
    • two target frameworks of one lock
    • the root lock plus a member project's lock, where each mark names the lock holding the other version
    • 13.0.9 / 13.0.10 precedence order
    • a single version, which is still attested
  • vendored_id_locked_at_another_version_is_unattested covers:
    • 13.0.1 + 13.0.3 locked: one ref, at 13.0.3
    • only 13.0.1 locked: the feed's 13.0.3 is the ref, with vendored_claim == Some(true)
    • 13.0.3.0 against the feed's 13.0.3, which is not "other"
  • non_normalized_version_spellings_are_one_version covers:
    • 13.0.3, " 13.0.3 " and 13.0.3.0 → one ref
    • conflicting hashes across spellings and locks → patched_ref_invalid, with the diagnostic naming both locks and spellings
    • one spelling unpinned → no lockfile basis
    • an empty version → diagnosed and unattested

Runs:

  • cargo test -p socket-patch-core --lib -- vex::discover:: formats::nuget vendor::nuget: 582 passed
  • The golden test committed_fixture_corpus_matches_golden passes, with no golden changes
  • cargo clippy --locked --workspace --all-features -- -D warnings: clean
  • cargo fmt --all -- --check: clean

Not done

  • The vendored writer refuses another locked version only before it first wires the config. Once wired, its in-sync check ignores other-version entries. Discovery follows the hosted writer and the NuGet build (that framework's restore fails) and withholds the statement, which can miss an attestation but never makes a false one.
  • CHANGELOG.md is untouched, per AGENTS.md.

Review findings, round 2 (/code-review high on 56bffb2, cloud routine)

Each test below fails on 56bffb2, as worked through from the code; I didn't run them there.

  1. Discovery trimmed resolved but the writers don't: confirmed and fixed. A padded 13.0.3 beside 13.0.3 counted as one version to discovery, so it was attested. The writers treat it as another version and refuse the lock. Discovery now keys the raw value, as the writers do, so the ref is withheld (vex_nuget_lock_other_version). The CLI_CONTRACT row is corrected. Test: non_normalized_version_spellings_are_one_version, extended.
  2. A vendored ref was attested over an unreadable lock: confirmed and fixed. Hosted emits no ref there, and the lock might hold another version the writers refuse. Vendored now matches hosted (owner decision 3). The lockfile_unparseable diagnostic still reports the lock. Test: vendored_unreadable_lock_and_feed_leaf_spelling.
  3. The vendored artifact leaf depended on whether the lock pinned it: confirmed and fixed. The ref now always names the feed's file as it is on disk. Test: same as 2.
  4. The unusable-version diagnostic named the root lock: fixed. It now names the locks that hold the entry.
  5. Pre-release ordering: fixed. Identifiers compare as SemVer 2 / NuGet do, so rc.2 sorts before rc.10. Test: precedence_orders_prerelease_identifiers.

Declined:

  • Trimming in the shared reader instead. The writers are ground truth, and NuGet itself treats the padded string as unparseable, so discovery now follows the writers rather than changing them.
  • Repeated sorts, the 4-tuple, and the redundant contains. These are performance and style only.

Tests (cloud run): cargo test -p socket-patch-core --lib -- vex::discover:: formats::nuget vendor::nuget patch::redirect::tests::nuget gives 616 passed, golden corpus included. -p socket-patch-cli --lib vex_sources passes. clippy and fmt are clean.


Note

Medium Risk
Changes attestation eligibility and NuGet ref discovery—wrong normalization or unattested handling could omit or over-attest patches; mitigated by extensive targeted tests and alignment with existing writer rules.

Overview
NuGet VEX discovery now matches the writers: governed packages.lock.json entries are keyed by normalize_nuget_version on trimmed resolved, so equivalent spellings (e.g. 13.0.3 vs 13.0.3.0) collapse to one ref; empty resolved is diagnosed and treated like another conflicting version. Only Socket-mapped package ids are indexed from locks.

When a lock pins the same id at another version beside the one the Socket source can serve (the *_nuget_lock_other_version case), refs stay for rollback / ledger liveness but are marked UnattestedKind::NugetLockOtherVersion and the plan surfaces vex_nuget_lock_other_version with guidance in vex_sources. Hosted: every ref for that mapping is checked against other locked versions; multiple versions withhold all of them. Vendored: a single feed nupkg defines the patched version as the sole ref (pinned when locked there); other locked versions appear only in the unattested detail, not as separate refs to missing artifacts.

CLI_CONTRACT.md updates the NuGet table and unattested-reference rules; the diagnostic catalog adds vex_nuget_lock_other_version. Core tests cover multi-TFM locks, root vs member locks, precedence ordering, vendored-only-wrong-version, and normalization edge cases.

Reviewed by Cursor Bugbot for commit 56bffb2. Configure here.


Generated by Claude Code

When a governed packages.lock.json locks a Socket-mapped NuGet id at a
version other than the patched one (another target framework, or another
project's lock), both writers refuse to wire it
(redirect_nuget_lock_other_version, vendor_nuget_lock_other_version):
the exact-id mapping routes every version to a source that serves only
the patched one, so that framework cannot restore. Discovery instead
emitted an attested ref per locked version, including the unpatched one.

Discovery now applies the writers' rule. Each ref of such a source is
still emitted (rollback, remove, list and the ledgers' liveness gates
keep seeing the wiring) but marked with the new
UnattestedKind::NugetLockOtherVersion, which the VEX plan omits with the
vex_nuget_lock_other_version note. The patched version comes from where
the files name it: the vendored feed's single nupkg; a hosted url names
only the uuid, so any two locked versions qualify there.

Lock versions are now keyed through normalize_nuget_version, as the
writers' locked_at / other_versions match them, so 1.0 and 1.0.0 are
one version with one pin check rather than two refs with independent
pins.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Vendored: the feed's single nupkg names the patched version, so it is
  the only ref, even when no lock entry is at it (keeping the ledger's
  vendored_claim live); other locked versions are reported in the
  unattested detail, never as refs pointing at nupkgs that don't exist.
- Each ref's other-version check excludes its own version: the detail
  and file name the lock holding the OTHER version, listed in NuGet
  precedence (13.0.9 before 13.0.10), and say "also lock" only when the
  ref's version is locked.
- Trim `resolved` before normalizing; an empty one is diagnosed and
  counts as another version (the writers would refuse it).
- The contentHash-conflict diagnostic names the locks and spellings.
- Only Socket-mapped ids are indexed; one push_ref helper marks refs.

Co-Authored-By: Claude Opus 5.5 <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.

✅ 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 56bffb2. Configure here.

@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review November 6, 2026 12:12
@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 6, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review at 56bffb2.

  • CI: 30/32 non-skipped checks green. The only failures are the main-wide e2e … pdm:: --ignored leg (module 'hishel' has no attribute 'SyncSqliteStorage', also red on main; Fix pdm e2e red on every PR: pin hishel<2 #2028 fixes it) and the ci-ok aggregate that follows from it.
  • Bugbot: reviewed 56bffb2; no unresolved findings.
  • Promoted from draft by the burn-down agent this run. No Slack announcement was sent this run (the Slack send action wasn't available), so there's no slack-announced marker yet.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief (for the human approver)

What it does: NuGet VEX discovery now keys lock entries by normalized version, so 13.0.3 and 13.0.3.0 count as one. It also applies the writers' other-version rule: when the governing locks lock a mapped id at another version, the refs are kept but marked NugetLockOtherVersion, and the VEX plan omits them (vex_nuget_lock_other_version). In vendored mode, the feed's single nupkg is now the only ref, instead of every locked version.

Risk: low-medium. Discovery only gets stricter: it adds unattested marks and narrows the vendored refs to what the feed actually serves. Ledger liveness is unchanged because the refs stay refs. PurlKey already uses the same normalization, and the new enum variant is handled in the only exhaustive match.

Look here

  • nuget.rs:389-422 load_lock: indexing, trim, empty resolved, normalization
  • nuget.rs:514-551 other_versions: the core rule
  • nuget.rs:713-760 emit_vendored: the feed-first branch
  • formats/nuget/lock.rs:103: the writers' rule it mirrors

Verified

  • I read the full diff and compared it with the base emit_vendored: with a lock, without one, with conflicting hashes, with no feed leaves, and in the in-memory view.
  • The new tests assert the marks, detail text, precedence and vendored_claim/hosted_claim, and would fail on the old code.
  • There is no new path surface: the feed dir join still goes through the uuid grammar. CHANGELOG.md is untouched.
  • CI: 30/32 non-skipped checks green. The only failures are the main-wide pdm/hishel leg and the ci-ok aggregate; Fix pdm e2e red on every PR: pin hishel<2 #2028 fixes both.
  • Bugbot: clean on this head.

Changes I made: none.

Open questions (non-blocking)

  • Discovery trims resolved before normalizing, but the writers' other_versions/locked_at do not. So for " 13.0.3 " beside 13.0.3, the writers refuse while discovery attests. NuGet never writes padded values, but the comment at nuget.rs:280-283 ("as the writers match it") isn't strictly true. Trim in the writers too, or reword the comment.
  • When the patched version is locked, the vendored artifact path is rebuilt with nupkg_leaf(...) (nuget.rs:734) instead of the leaf found in the feed. The two only differ for a non-normalized feed file name.

Auto-merge is not armed yet, because CI is red on the main-wide pdm failure. Once #2028 lands and this goes green, the next final-reviewer run arms it, and approving then sends it straight to the merge queue.


Generated by Claude Code

Second review round on NuGet other-version discovery.

- Discovery keyed lock versions by `normalize_nuget_version` of the
  TRIMMED `resolved`, while both writers normalize the raw value: a
  padded ` 13.0.3 ` beside `13.0.3` was one version to discovery (and
  attested) but another to the writers (which refuse the lock). It is
  keyed raw now, so the ref is withheld (`vex_nuget_lock_other_version`).
- A vendored ref was still emitted, attested and unpinned, when the lock
  could not be read; hosted emits none. Vendored now matches hosted.
- A vendored ref named `nupkg_leaf(id, lock spelling)` when locked but
  the feed's real file otherwise; it always names the feed's file.
- The unusable-version diagnostic named the root lock even when only a
  member lock held the entry; it names the locks that do.
- Pre-release labels order by identifier (numeric ones numerically), so
  the lock an other-version mark points at follows NuGet precedence.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants