Repository navigation
feat(manifest): generate Socket facts for .NET solutions and projects - #1678
Jeppe Fredsgaard Blaabjerg (jfblaa) wants to merge 4 commits into
Conversation
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix is ON, but none of the reported issues met your autofix severity threshold.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 02fbf71. Configure here.
| return; | ||
| } | ||
| foreach (var tfm in inner) EmitBuild(info, project, tfm, tfm); | ||
| } |
There was a problem hiding this comment.
Sidecar emits excluded target frameworks
Medium Severity
EmitPaths still writes compile context when --include-configs / --exclude-configs filtered out every target. A single-framework project always takes the fallback because TargetFramework is set, and a multi-targeting project whose aliases were all skipped falls through to an outer/LegacyFrameworkName build. --reach sidecars then carry sources and references for frameworks the run was told not to read.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 02fbf71. Configure here.
There was a problem hiding this comment.
[agent] Fixed in fc43d03 - see the reply on jdalton's thread on the same line.
| } | ||
| if (File.GetLastWriteTimeUtc(cachePath) < _restore.StartedUtc) return cache.Success; | ||
| ReportLogErrors(info, cache); | ||
| return true; |
There was a problem hiding this comment.
Failed restore reuses stale assets
Medium Severity
When this run's dotnet restore fails or is killed, RestoredByThisRun still treats a pre-existing project.nuget.cache whose timestamp predates the restore as success and ReadAssets walks that older project.assets.json. --ignore-unresolved then downgrades the restore/timeout failure and the scan can upload a graph from a previous restore.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 02fbf71. Configure here.
There was a problem hiding this comment.
[agent] Fixed in fc43d03 - see the reply on jdalton's thread on this function.
02fbf71 to
7b97ebc
Compare
7b97ebc to
de78286
Compare
| var inner = frameworks.Where(f => _configs.Matches(new[] { f })).ToList(); | ||
| var single = project.GetPropertyValue("TargetFramework"); | ||
| if (inner.Count == 0 || !string.IsNullOrEmpty(single)) { | ||
| EmitBuild(info, project, null, string.IsNullOrEmpty(single) ? LegacyFrameworkName(project) : single); | ||
| return; | ||
| } | ||
| foreach (var tfm in inner) EmitBuild(info, project, tfm, tfm); | ||
| } |
There was a problem hiding this comment.
The framework list is filtered at some call sites and raw at others. This also needs to preserve single-framework projects while omitting targets excluded by the config filter.
| var inner = frameworks.Where(f => _configs.Matches(new[] { f })).ToList(); | |
| var single = project.GetPropertyValue("TargetFramework"); | |
| if (inner.Count == 0 || !string.IsNullOrEmpty(single)) { | |
| EmitBuild(info, project, null, string.IsNullOrEmpty(single) ? LegacyFrameworkName(project) : single); | |
| return; | |
| } | |
| foreach (var tfm in inner) EmitBuild(info, project, tfm, tfm); | |
| } | |
| var single = project.GetPropertyValue("TargetFramework"); | |
| if (Frameworks(project).Count == 0) { | |
| var config = string.IsNullOrEmpty(single) ? LegacyFrameworkName(project) : single; | |
| if (_configs.Matches(new[] { config })) EmitBuild(info, project, null, config); | |
| return; | |
| } | |
| foreach (var tfm in frameworks.Where(f => _configs.Matches(new[] { f }))) { | |
| EmitBuild(info, project, tfm, tfm); | |
| } | |
| } |
| using (var stream = File.OpenRead(cachePath)) { | ||
| cache = CacheFileFormat.Read(stream, NullLogger.Instance, cachePath); | ||
| } | ||
| if (File.GetLastWriteTimeUtc(cachePath) < _restore.StartedUtc) return cache.Success; |
There was a problem hiding this comment.
After this run’s restore fails, a cache file from before the restore cannot prove the existing assets are current. Return false at the stale-cache check so --ignore-unresolved cannot emit a graph from a previous restore.
| if (File.GetLastWriteTimeUtc(cachePath) < _restore.StartedUtc) return cache.Success; | |
| if (File.GetLastWriteTimeUtc(cachePath) < _restore.StartedUtc) return false; |
There was a problem hiding this comment.
[agent] Fixed in fc43d03 - took the suggestion. NuGet doesn't rewrite the cache of a project it finds up to date either (NoOpRestoreResult.CommitAsync), so after a failed restore those projects now count as not restored too, and each gets an unscannable record saying so. That's fail-closed, and it only matters under --ignore-unresolved. #1681 adds a smoke check (cd920f4) that fails on the old code.
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
There was a problem hiding this comment.
Simplification pass: reuse, simplification, efficiency and altitude. It doesn't look for correctness bugs. The specifics are inline. One structural point doesn't fit on a single line, and I'd do it as a follow-up rather than in this PR:
An ecosystem registry. Adding .NET meant hand-editing about 15 places across 11 files:
BUILD_TOOLSandROOT_ECOSYSTEMS, in different orders- the
BuildToolliteral copy inscripts/facts.mts - the
run.mtsswitch and ternary - the
resolveEcosystemConfigandrunEcosystemWizardif-chains - the
detect-manifest-actionsbooleans and hard-coded file names that copyBUILD_FILES - the per-ecosystem blocks in
generate_auto_manifest - the
GeneratableManifestsreset list inhandle-create-new-scan.mts socket-jsontypes and merge- the
setupXwizards - the tool list in the
coana-fixmessage
Several of these are plain arrays or chains that fall through to maven or sbt, so the compiler doesn't catch a forgotten entry. A missing reset line in handle-create-new-scan.mts would bring back the double-generation race described in its comment.
A Record<BuildTool, EcosystemDescriptor> would fix this. Each descriptor would hold the label, purl type, bin default, socket.json key, opts key, report dialect, workspaces and facts runners, setup wizard, an optional preflight, and an optional entry-members hook for solution coverage. The arrays, labels and GeneratableManifests would be derived from its keys. The next ecosystem would then be one entry, and several inline comments here would collapse into it. build-tool.mts already points this way with PURL_TYPE_BY_TOOL and BUILD_FILES.
de78286 to
fc43d03
Compare
`socket manifest dotnet` writes `<entry>.socket.facts.json` per solution, plus one per project no solution builds, from the user's own `dotnet restore`: a bundled C# tool evaluates projects in-process on the selected SDK (MSBuildLocator) and reads project.assets.json with NuGet's own API. PackageReference (central package management, lock files, multi-targeting, runtime targets) and packages.config are supported, for C#, F# and VB. --auto-manifest, `socket manifest auto`, dynamic SBOM inference and the setup wizards detect .NET builds and skip them with a warning where no .NET SDK runs. --reach sidecars carry each project's compiler inputs per target framework from a design-time build, and scan and fix pass Coana --use-only-socket-facts-artifacts when NuGet facts are present. Resolution, restore and design-time build failures fail closed.
- Honour --include-configs/--exclude-configs in the per-framework build context the sidecar carries, for single- and multi-targeting projects. - After a failed restore, treat a project.nuget.cache older than the restore as not restored: NuGet leaves an up-to-date project's cache untouched, so it cannot tell such a project from one the restore never reached. - Keep the coverage rule in one isRootCovered, cache solution member listings, and probe the SDK once per global.json. - Build every script's options from one list, type the per-build projectInfo records, and drop dead fields and parameters. - Classify NuGet failures by error code only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The .NET tool's compile-time closure resolved System.Text.Json 6.0.0 (CVE-2024-43485) and System.Formats.Asn1 5.0.0 (CVE-2024-38095). Neither ships, since the shared framework supplies both at run time, but the lock file still carried them. Reference the patched versions compile-only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
341d2c5 to
593b5af
Compare
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
There was a problem hiding this comment.
Second simplification pass, on the current head. I filtered it against the earlier threads, the follow-ups already agreed (ecosystem registry, preflight hook, done record, TS-compiled config globs, package caching, parallel design-time builds) and #1682's wording changes. What's left is inline.
One design point doesn't fit on a line. In a repository without a solution, every standalone project root gets its own tool start and its own dotnet restore. Projects reached from several roots are evaluated again each time, and under --reach design-time built again too. If the tool took several entries in one run, it could restore them through one temporary filter and share one build session while still writing a records file per entry. That overlaps the one-start-per-solution follow-up, so it may belong there.
| const dir = await realpathOrResolved(cwd) | ||
| const covered = new Set<string>() | ||
| let last: RunManifestFactsOutcome | ||
| for (const entry of entries) { |
There was a problem hiding this comment.
The coverage predicate is shared now, but the loop around it is still a trimmed copy of runEcosystemCandidates: the covered set, isRootCovered, runManifestFacts, stopping on null, then addCoveredBuildFiles. Pulling that loop body out as a helper over roots that both call would keep the two from drifting. The callers already spread EcosystemBuildConfig fields into this function one by one, so the helper could take the config whole.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - the per-root step (isRootCovered, runManifestFacts, addCoveredBuildFiles) is now buildUncoveredRoot in generate-recursive-manifests.mts, taking the EcosystemBuildConfig whole. Both loops call it, and convertDotnetToFacts takes config instead of the spread fields.
| ext.toLowerCase() === '.sln' ? '.slnx' : '.sln', | ||
| ) | ||
| const hasTwin = existsSync(path.join(root.dir, twin)) | ||
| if (!covered.size && !hasTwin) { |
There was a problem hiding this comment.
In the setup wizard, covered is shared across ecosystems (setup-recursive-manifest-config.mts:871), and ROOT_ECOSYSTEMS puts dotnet last. Once any Maven, Gradle or sbt root is covered, covered.size is non-zero, so every solution pays for a --members tool start, though a JVM build file can never cover a .NET project. Gating on whether covered holds a .NET project file (isBuildFile('dotnet', …), or a per-tool count) skips those starts without waiting for the one-start-per-solution follow-up.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - went one level down instead: the recursive generator and the setup wizard now keep a covered set per tool, so a Maven, Gradle or sbt build can no longer make a solution list its members, and isSolutionCovered keeps its cheap covered.size check. #1681 adds a recursive-run test with a covered POM next to a solution that fails on the shared set.
| // The facts files' per-project classpaths, outside the repository. | ||
| sidecarFile: string | undefined | ||
| // The facts files' per-project classpaths, written outside the repository. | ||
| sidecar: { contents: ResolvedPathsSidecar; file: string } | undefined |
There was a problem hiding this comment.
contents keeps the parsed sidecar alive for the whole fix loop, because factsSlot.generated lives until withTmpDir returns. With --with-files, the .NET sidecar's references, sources and compile targets can be large. Its only reader is the one useOnlySocketFactsArtifactsFlags call in coana-fix.mts. Computing the flags here and storing { file, flags } would let it be collected right after it's written. That still holds once the legacy-flag branch goes, since the remaining flag still needs the purl types.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - generateSocketFactsForFix computes the Coana flags when it writes the sidecar and returns { artifactsFlags, sidecarFile }, so the parsed sidecar is not kept for the fix loop.
| verbose, | ||
| } = cli.flags | ||
|
|
||
| function useDefault<T>(flag: string, value: T): T { |
There was a problem hiding this comment.
This rebuilds the dotnet defaults that resolveEcosystemConfig('dotnet', …) (generate-recursive-manifests.mts:396) already produces, and the two already differ. Here bin is checked for truthiness and there with ??, so bin: "" in socket.json resolves differently. Starting from resolveEcosystemConfig and laying the CLI flags over it would leave one place for the defaults. The "Using default --x" lines can come from comparing each flag with the config.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - the command starts from resolveEcosystemConfig('dotnet', …) and lays the given flags over it, so the defaults live in one place; the "Using default" lines come from comparing each flag with socket.json. bin now uses || there, so bin: "" resolves the same way in both paths.
| // dotnet: the target framework's NuGet short folder name, the runtime | ||
| // identifier of a runtime-specific target, and the framework's alias in | ||
| // the project; empty otherwise. | ||
| framework: string |
There was a problem hiding this comment.
These three are always set together, only for dotnet, and are '' on every JVM root. buildFrameworks tells the cases apart with filter(r => r.framework) (assemble.mts:516). An optional target?: { alias; framework; runtime } on RawRoot and PerRoot, set when the root record carries the extra fields, would put that in the type and turn the copy at assemble.mts:177-179 into one field.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - RawRoot and PerRoot carry target?: { alias, framework, runtime }, set only when the root record has a framework; buildFrameworks filters on it and the copy is one field.
| ): Promise<CResult<{ canceled: boolean }>> { | ||
| if (ecosystem === 'dotnet') { | ||
| return await setupDotnet( | ||
| config as NonNullable< |
There was a problem hiding this comment.
Nit: this four-level NonNullable<…['manifest']['dotnet']> is one of nine spellings across the two setup files. A ManifestDefaults<K> alias in utils/socket-json.mts would shorten all of them.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - added ManifestDefaults<K> to utils/socket-json.mts and used it for all nine.
| var spec = lockFile?.PackageSpec; | ||
| if (lockFile == null || spec == null) { | ||
| EmitProject(info, project); | ||
| _records.Unscannable("", $"could not read {Rel(assetsPath)}", info.Key); |
There was a problem hiding this comment.
The three exits in this function repeat EmitProject(info, project); _records.Unscannable("", …, info.Key); return Array.Empty<string>();, and the first adds the _notRestored branch. A local function such as string[] Unscannable(string detail) would leave each exit as one line.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - a local Unscannable(detail) covers the three exits; a null detail stands for the failed-restore case that ReportRestoreErrors reports.
| if (!string.IsNullOrEmpty(value)) _records.ProjectInfo(info.Key, config, property, value); | ||
| } | ||
| // Unrecognized, the alias stands in. | ||
| if (ShortFrameworkName(instance.GetPropertyValue("TargetFrameworkMoniker"), instance.GetPropertyValue("TargetPlatformMoniker")) is { } framework) { |
There was a problem hiding this comment.
Nit: a ShortFrameworkName(ProjectInstance) overload, like the Project one at :535, would shorten this call and match it.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - added the ProjectInstance overload.
| } | ||
| } | ||
|
|
||
| private void Fail(string coord, string detail, string config, string projectKey = "") { |
There was a problem hiding this comment.
Fail only forwards to _records.Failure, adding the projectKey default, while other call sites use _records.* directly. Putting the default on RecordsWriter.Failure would remove it.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - RecordsWriter.Failure has the projectKey = "" default and Fail is gone.
| DateTime StartedUtc, | ||
| List<RestoreError> Errors | ||
| ) { | ||
| public bool Succeeded => !TimedOut && ExitCode == 0; |
There was a problem hiding this comment.
A timeout is recorded twice, as ExitCode = -1 (:67) and as TimedOut. The only reader of ExitCode (FactsRunner.cs:313) is already inside a !_restore.TimedOut branch. An int? ExitCode that is null on timeout would leave one source of truth: Succeeded => ExitCode == 0.
There was a problem hiding this comment.
[agent] Fixed in e10e2dd - ExitCode is int?, null on timeout; TimedOut and Succeeded derive from it.
- Share the per-root build step (coverage check, run, covered files) between the recursive generator and `manifest dotnet`, which now takes the ecosystem config whole and starts from resolveEcosystemConfig. - Keep covered build files per tool, so JVM coverage no longer makes every solution list its members. - Keep only Coana's flags and the sidecar path for `socket fix`, not the parsed sidecar. - Carry a root's target framework as one optional field; reuse the build and root nodes when assembling per-framework sidecar data. - Add getOrInsert, ManifestDefaults and wizard answer helpers. - C# tool: nullable restore exit code, Failure's default project key, one unscannable exit in ReadAssets, ShortFrameworkName(ProjectInstance). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[agent] On the design point in the second review (several standalone project entries in one tool start, sharing one restore filter and one build session): agreed, and I'd fold it into the one-start-per-solution follow-up, since both need the tool to take several entries and write a records file per entry. Not changing it in this PR. |
LLM Description written by Claude Code:claude-opus-5-5
Warning
Do not merge until a Coana release accepts
--use-only-socket-facts-artifactsand NuGet sidecar entries, and this PR bumps the pinned Coana to it. JVM-only scans are unaffected: they keep passing--maven-use-only-socket-facts. The SBOM-only path (no--reach, nosocket fix) does not depend on Coana.What
.NET/NuGet support in the Socket facts pipeline, at parity with Maven/Gradle/sbt. Revives the approach of #1404 on top of the current facts format.
socket manifest dotnet [DIR|FILE]writes<entry>.socket.facts.json: one per solution (.sln,.slnx,.slnf), plus one per project no solution builds. Same shape as the JVM facts files: nuget purls (no namespace), one dependency tree per project, project ids by build-root-relative project path,manifestFilesnaming the project file and anyDirectory.Packages.props/imports, packages.config or lock file that declare its dependencies.dotnet restore(same host, SDK/global.json, env, nuget.config, credential providers), so whatever restores for the user restores here. The bundled C# tool evaluates projects in-process via MSBuildLocator on that SDK, with restore's import handling and the solution's active configuration, and readsproject.assets.jsonwith NuGet's own API. Ships no NuGet/MSBuild assemblies; runs on SDK 6 through 11.VersionOverride,GlobalPackageReference, lock files/locked mode, multi-targeting, runtime-specific targets; packages.config; C#, F#, VB.developmentDependency(build targets can copy native files, asMicrosoft.Data.SqlClient.SNI's do). Everything a test project resolves is dev.obj/), a failed design-time build under--reach, and tool crashes are blocking failures (NuGet dialect in the resolution report),--ignore-unresolvedas for the JVM tools.--auto-manifest,socket manifest auto, dynamic SBOM inference and both setup wizards detect .NET builds; where no .NET SDK runs, they skip .NET with a warning and the project files are scanned as before. A project counts as covered wherever it lives, and--exclude-pathsapplies to a solution's projects wherever they live (scan-root anchoring from fix(manifest): anchor --exclude-paths at the scan root for every build root #1679, below this PR in the stack); a solution is skipped once every project it builds is covered, and an unreadable.slnx/.slnis skipped with a warning when its twin in the other format is readable.socket.jsongainsdefaults.manifest.dotnet.--reachsidecars give each project one entry per target framework (and runtime), read from a design-time build as Roslyn's MSBuildWorkspace runs it: sources in compile order (generated ones included), references (framework reference packs included), analyzers, preprocessor symbols, language version and nullable context, output, framework references, implicit usings, direct dependencies and classpath; and each package its runtime and compile assemblies per framework. Reachability andsocket fixpass Coana--use-only-socket-facts-artifacts maven,nugetwhen NuGet facts are present.Testing
Tests are in #1681 and the help text and comment wording in #1682, both above this PR in the stack. Together: unit suites pass, and
run-compat.sh dotnetpasses on SDKs 6.0.428, 7.0.410, 8.0.414, 9.0.315, 10.0.301 and 11.0.100-rc.3. Run by hand over public .NET repositories (solutions up to 272 projects), every facts file matcheddotnet list packageexactly.Limitations
--reach, assemblies come from an existing packages folder, whichdotnetcannot restore.$(SolutionName), solution configuration mapping) is read in the first build that reaches it.NuGetAudit=false, so a build that turns NuGet audit warnings into errors restores here but not natively.--reachruns each project's design-time build in-process (its build targets run, the compiler does not), which on a 150-project synthetic solution took the tool from 12 s to 21 s. A design-time build that fails where the project would not compile either (WPF off Windows; .NET Framework reference assemblies missing off Windows on older SDKs) fails the run;--ignore-unresolvedcontinues with its evaluated sources and symbols and abuildErrorin the sidecar.--auto-manifeston a repository with a .NET solution or project needs a workingdotnet restorewherever a .NET SDK runs, and fails the scan otherwise, as for Gradle/Maven/sbt;defaults.manifest.dotnet.disabledopts out.developmentDependencydeclaration count as prod even when they only serve the build.🤖 Generated with Claude Code