Skip to content

fix(dotnet): resolve packages.lock dependencies deterministically - #5210

Open
spiffcs wants to merge 2 commits into
mainfrom
dotnet-packages-lock-per-tfm-deps
Open

fix(dotnet): resolve packages.lock dependencies deterministically #5210
spiffcs wants to merge 2 commits into
mainfrom
dotnet-packages-lock-per-tfm-deps

Conversation

@spiffcs

@spiffcs spiffcs commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Description

Follow-up to #5143. See #5211 for more details.

While reviewing that fix I saw that the packages.lock.json parser resolves dependency edges against a global map of every package in the lockfile.

This resolution would ignore which target framework the edge was declared in.

Multi-targeted projects routinely resolve the same package to different versions per framework. Because the old code looked up the declared version globally and, on a miss, fell back to "any package with this name" via non-deterministic map iteration, edges could point at the wrong version or flip between runs.

Given a lockfile that pins log4net to 2.0.5 under net8.0 and 1.2.15 under netstandard2.0:

Before

Dependency edges were resolved from the global package map. The netstandard2.0 edge (log4net 1.2.15) matched exactly, but a package could fall through to findPkgByName, which returned whichever name match Go's map iteration happened to yield first.

After

Each edge resolves within its declaring framework, so net8.0log4net 2.0.5 and netstandard2.0log4net 1.2.15 happens deterministically.

Changes:

  • Sort framework, package, and dependency keys so relationship ordering is stable.
  • Deduplicate relationships by (child, parent) package ID, since frameworks that resolve to the same versions would otherwise emit the same edge more than once.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have added unit tests that cover changed behavior
  • I have tested my code in common scenarios and confirmed there are no regressions
  • I have added comments to my code, particularly in hard-to-understand sections

Issue references

Found during review of #5143

…ramework

Signed-off-by: Christopher Phillips <32073428+spiffcs@users.noreply.github.com>
… target frameworks

Signed-off-by: Christopher Phillips <32073428+spiffcs@users.noreply.github.com>
@spiffcs
spiffcs force-pushed the dotnet-packages-lock-per-tfm-deps branch from ac8570f to c60cb64 Compare August 21, 2026 20:21

@CAOShurong CAOShurong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

End-to-end verification on the current head (c60cb64), Windows, syft built from the exact head with the issue's multi-target scenario (Newtonsoft.Json Direct under net8.0 / Transitive under netstandard2.0, Serilog pulled transitively via MyLogAdapter):

  • determinism, 100 scans of the same fixture: main flips (88 Direct / 12 Transitive), this head is 100/100 "Direct" — the reported nondeterminism is gone and Direct-wins is stable;
  • relationships resolve per-framework correctly and dedupe across frameworks: the fixture yields exactly one "Serilog dependency-of MyLogAdapter" edge, and Serilog itself is reported Transitive as every framework sees it;
  • package order and relationship order are stable across runs.

One scope note (matches what the PR already implies): metadata.requested from the lockfile is still not surfaced (metadata exposes name/version/type/contentHash only), so the second observation in #5211 remains open beyond this PR — worth a tracking note so it isn't lost when this merges.

@CAOShurong

Copy link
Copy Markdown
Contributor

Independent end-to-end verification of #5210 (maintainer fix for #5211), run 2026-08-24 ~02:20+08:00.

Setup

  • Repro from the issue: a packages.lock.json where Newtonsoft.Json 13.0.3 is Direct under net8.0 and Transitive under netstandard2.0, scanned with syft scan dir:. --select-catalogers dotnet -o syft-json.
  • Baseline = local build of main @ bf82010f (Windows/amd64, go1.26.6). Candidate = local build of PR head c60cb640. Both built clean; the parser file is untouched between them, so the baseline reflects upstream behavior.
  • Parser unit tests were run in a throwaway worktree at the PR head; it has been removed (the repo working tree is back on our own branch, clean).

Baseline (main): nondeterminism confirmed on Windows too

60 scans of the byte-identical input:

  • 50 runs → "type": "Direct"
  • 10 runs → "type": "Transitive"

This reproduces the issue's flip-rate (~17% here vs ~16% in the issue) outside Linux, so the randomized-map-order root cause is platform-independent. Also confirmed on main: metadata.requested is dropped entirely — the output metadata carries only name/version/contentHash/type.

Candidate (#5210 head c60cb64): deterministic

Same loop, same machine: 60/60 runs → "type": "Direct", zero flips across repeated invocations of separate processes (each run is a fresh process, so map iteration order differs per run).

Tests

  • go test ./syft/pkg/cataloger/dotnet/ -run 'TestParseDotnetPackagesLock' including the new TestParseDotnetPackagesLock_multipleTargetFrameworks: PASS, also PASS at -count=20.
  • The three failures elsewhere in that package (TestCataloger_Globs, TestCataloger, TestDotnetDepsCataloger_regressions/libman_support) are environment-only: all three fail identically on unmodified main because they shell out to docker, which isn't installed here. They are unrelated to this change.

One observation, not a blocker

The fix resolves the determinism and Direct-preference halves of the issue cleanly. The lossiness half remains open by design: after stabilization the SBOM reports only "type": "Direct" for Newtonsoft.Json even though one target framework lists it as Transitive, and requested still isn't captured. The issue itself frames this as acceptable ("we can say syft doesn't support the transitive entries for this cataloger yet"), so this looks intentional for now — flagging only so the tradeoff is on record from an independent run.

Nice-to-have if you want belt-and-suspenders coverage: a test asserting that two parses of the multi-framework fixture produce identical package metadata ordering, which would catch any future regression into map-order dependence even before golden-file comparison.

Verified as an independent third party; no code changes proposed — the PR looks correct and ready from this side.

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

Labels

bug Something isn't working needs-discussion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants