Repository navigation
Stop attesting NuGet versions the writers refuse - #2007
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
Conversation
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>
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ 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.
|
Ready for review at 56bffb2.
Generated by Claude Code |
Final review brief (for the human approver)What it does: NuGet VEX discovery now keys lock entries by normalized version, so 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. Look here
Verified
Changes I made: none. Open questions (non-blocking)
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>
Problem
Sometimes a governed
packages.lock.jsonlocks 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:rewrite_nuget→redirect_nuget_lock_other_versionnuget_prelude→vendor_nuget_lock_other_versionThe 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 rawresolved.trim(), while the writers match throughnormalize_nuget_version. As a result,13.0.3and13.0.3.0became two refs, each with its own pin check.Fix
normalize_nuget_version(resolved.trim()).PurlKey).resolvedgetspatched_ref_invalid. The writers would count it as another version, so it withholds the statement the same way.UnattestedKind::NugetLockOtherVersion. The VEX plan then omits it withvex_nuget_lock_other_version. The note names the other versions in NuGet precedence order, and the lock that holds the first of them.<idLower>.<versionNorm>.nupkgnames 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'svendored_claimstays 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.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) makeshosted_claim/vendored_claimreturnSome(false)for the ledger record of the patched version. Thenscan,vendor --check,rollbackandremovewould treat Socket's own wiring, which is still present, as reverted. Re-runningscan/vendorcan't clear that either.Unattestedis meant for exactly this case.Review findings (
/code-review high)vendored_claimon the real artifact returnedSome(false). The feed's artifact is now the ref, unpinned and unattested. The test assertsvendored_claim == Some(true).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 lockcontentHashfails a restore from the competing source anyway. This module's non-goals already say so.local_record_by_uuid) or from the API, names another package, so it is gatedrecord_mismatch. Without a record it is gatedrecord_unavailable.hosted_claimfinds no ref wiring 13.0.3.Discoveryfield and a plan pass after record resolution, with no change in behavior. CLI_CONTRACT.md now documents this split." 13.0.3 "next to13.0.3used to read as two versions. Values are now trimmed before normalizing, and an empty value is diagnosed.packages.lock.json, src/App/packages.lock.json: Newtonsoft.Json 13.0.3 / 13.0.3.0 ….read_dirof a single uuid directory.push_refhelper 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_unattestedcovers:vendored_id_locked_at_another_version_is_unattestedcovers:vendored_claim == Some(true)13.0.3.0against the feed's13.0.3, which is not "other"non_normalized_version_spellings_are_one_versioncovers:13.0.3," 13.0.3 "and13.0.3.0→ one refpatched_ref_invalid, with the diagnostic naming both locks and spellingsRuns:
cargo test -p socket-patch-core --lib -- vex::discover:: formats::nuget vendor::nuget: 582 passedcommitted_fixture_corpus_matches_goldenpasses, with no golden changescargo clippy --locked --workspace --all-features -- -D warnings: cleancargo fmt --all -- --check: cleanNot done
Review findings, round 2 (
/code-review highon56bffb2, cloud routine)Each test below fails on
56bffb2, as worked through from the code; I didn't run them there.resolvedbut the writers don't: confirmed and fixed. A padded13.0.3beside13.0.3counted 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.lockfile_unparseablediagnostic still reports the lock. Test:vendored_unreadable_lock_and_feed_leaf_spelling.rc.2sorts beforerc.10. Test:precedence_orders_prerelease_identifiers.Declined:
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::nugetgives 616 passed, golden corpus included.-p socket-patch-cli --lib vex_sourcespasses. 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.jsonentries are keyed bynormalize_nuget_versionon trimmedresolved, so equivalent spellings (e.g.13.0.3vs13.0.3.0) collapse to one ref; emptyresolvedis 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_versioncase), refs stay for rollback / ledger liveness but are markedUnattestedKind::NugetLockOtherVersionand the plan surfacesvex_nuget_lock_other_versionwith guidance invex_sources. Hosted: every ref for that mapping is checked against other locked versions; multiple versions withhold all of them. Vendored: a single feednupkgdefines 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