Repository navigation
fix(scan): make every discovered ecosystem facts-only under --dynamic-sbom-inference - #1696
Conversation
…-sbom-inference Coana was told NuGet artifacts come only from facts files when some .NET build root happened to produce facts, and Maven always. Which raw manifests reachability and fix trusted therefore depended on what other build roots produced. Every ecosystem a build root was discovered for is now facts-only, generated or not; Maven alone keeps the older flag. When Coana consumes the facts, a .NET build root without a usable SDK now fails before any build runs instead of silently losing its dependencies, reachability fails clearly when no build root produced facts (Coana refuses such a run), and roots skipped in socket.json get a warning that the analysis leaves them out. Callers now name the facts consumer (fix or reach) instead of passing withFiles/sidecar flags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![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 1 potential issue.
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 af3e558. Configure here.
Ignore every gradle-compat fixture's .gradle/ dir, not only project/'s, so local compat runs can't be committed again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
There was a problem hiding this comment.
This review only looks for simplifications; it doesn't check correctness. I have no efficiency concerns: dotnetSdkVersion is memoized per (bin, global.json), so the new SDK probe doesn't spawn dotnet --version any more often than before. Comments marked "Nit" are optional.
| continue | ||
| } | ||
| if ( | ||
| if (ecosystem === 'dotnet' && requireDotnetSdk) { |
There was a problem hiding this comment.
The loop already has a 'skippedNoSdk' outcome for this case (line 236). Could this push that outcome, with the reason, and let runDynamicSbomInference decide after the loop that it's fatal? That's how it already handles 'failed' and 'skippedDisabled'.
That would remove requireDotnetSdk from two signatures. It would also remove the ecosystem === 'dotnet' check that now appears twice, each time with a different probe. The test would then no longer need to mock both noDotnetSdkReason and skipWithoutDotnetSdk.
The trade-off is that JVM roots would build before the failure. Today the check fails before building anything only because BUILD_TOOLS lists dotnet first (discover-manifest-roots.mts:20). If failing fast matters, a smaller step is one branch that probes once with noDotnetSdkReason and then either throws or warns and skips. The warn-once part could be split out of skipWithoutDotnetSdk for that.
There was a problem hiding this comment.
[agent] Fixed in d283907 - one probe, one branch. After discussing it, we went further: --dynamic-sbom-inference (every consumer, including a plain upload) and its setup wizard now fail closed when no .NET SDK runs, since the SDK is needed to generate the facts at all. So requireDotnetSdk and skippedNoSdk are gone. It still fails before any build runs. --auto-manifest keeps the lenient warn-and-skip through skipWithoutDotnetSdk.
| '- Install the .NET SDK, or point defaults.manifest.dotnet.bin in socket.json at one.', | ||
| '- Or leave it out with --exclude-paths, or with defaults.manifest.dotnet.disabled in socket.json.', |
There was a problem hiding this comment.
These next steps say a bare socket.json and never mention the setup command. The existing DSI hints (proceedHint in resolution-report-render.mts) name the specific file and point at DSI_SETUP_COMMAND, which this file already imports. This root's socket.json path is already built a few lines below (displayPath(path.join(dir, SOCKET_JSON))), so it could be hoisted and used here. The same applies to "Re-enable the builds that socket.json skips" in run-dynamic-sbom-inference.mts.
There was a problem hiding this comment.
[agent] Fixed in d283907 - the .NET next steps name this root's socket.json and point at socket manifest setup --dynamic-sbom-inference, and so does the re-enable remedy in run-dynamic-sbom-inference.mts.
| ? useOnlySocketFactsArtifactsFlags(resolvedPathsSidecar) | ||
| // Only dynamicSbomInference sets these, and sidecarPath is always set above | ||
| // then; Coana rejects this flag without one. | ||
| ...(factsOnlyTypes?.length |
There was a problem hiding this comment.
The sidecar file above is written when reachabilityOptions.dynamicSbomInference is set, but this flag is added when factsOnlyTypes is set. Only the comment keeps the two in step. Gating the sidecar write on factsOnlyTypes?.length as well, or passing { factsOnlyTypes, resolvedPathsSidecar } as one option, would make the flag and the sidecar it needs come from the same input.
There was a problem hiding this comment.
[agent] Fixed in d283907 - the sidecar is now written whenever factsOnlyTypes is non-empty, the same input the flag reads.
| sidecar, | ||
| withFiles, | ||
| }: { | ||
| consumer: FactsConsumer | undefined |
There was a problem hiding this comment.
consumer drives five behaviours: withFiles, sidecar accumulation, requireDotnetSdk, the warning about disabled roots, and the throw when no facts were produced. undefined acts as a third, unnamed mode: a DSI upload without reach.
Consider giving that mode a name (e.g. 'upload') so no behaviour depends on undefined. At the least, consumer?: would be better than a required parameter that can be undefined, and it would remove the explicit consumer: undefined in the tests.
Also, the rule at line 36 that Coana refuses a facts-only run without facts is a Coana constraint. It would sit better next to where the Coana flags are built than in this shared module.
There was a problem hiding this comment.
[agent] Fixed in d283907 - the upload-only mode is now named 'upload', so no behaviour depends on undefined. I left the no-facts check where it is: it needs the outcomes (disabled roots shape the remedy, and #1697 adds the narrowed selection), and it has to fail before performReachabilityAnalysis uploads the manifests.
| ): void { | ||
| const hasDisabled = outcomes.some(o => o.status === 'skippedDisabled') | ||
| // Coana (15.14.1) refuses a facts-only reachability run without any facts. | ||
| if (consumer === 'reach' && !outcomes.some(o => o.status === 'generated')) { |
There was a problem hiding this comment.
Nit: factsPaths is computed just before this function is called, so it could take factsPaths.length > 0 instead of working out from outcomes again whether any facts were generated.
There was a problem hiding this comment.
[agent] Fixed in d283907 - checkFactsForCoana takes hasFacts from factsPaths.
| let scanTargets = targets | ||
| // Sidecar forwarded to reachability; populated by dynamic SBOM inference. | ||
| let resolvedPathsSidecar: ResolvedPathsSidecar | undefined | ||
| let factsOnlyTypes: PURL_Type[] | undefined |
There was a problem hiding this comment.
Nit: both lets are assigned only from dynamicResult. A single let dynamicResult: DynamicSbomInferenceResult | undefined would do, with dynamicResult?.factsOnlyTypes and dynamicResult?.resolvedPathsSidecar at the call.
There was a problem hiding this comment.
[agent] Fixed in d283907 - one dynamicResult in handle-scan-reach.mts. handle-create-new-scan.mts keeps its own resolvedPathsSidecar, since --auto-manifest merges into it.
| return { projects: [], components: [{ type, name: 'x', id: 'x' }] } | ||
| } | ||
|
|
||
| describe('useOnlySocketFactsArtifactsFlags', () => { |
There was a problem hiding this comment.
These tests could be one it.for table over { types, expected }, with rows for ['maven'], ['nuget', 'maven'] and ['nuget'], named '$types -> $expected'.
There was a problem hiding this comment.
[agent] Fixed in d283907 - one it.for table named $types -> $expected, moved with the function to src/utils/coana.test.mts.
| }) | ||
|
|
||
| describe('performReachabilityAnalysis --maven-use-only-socket-facts gating', () => { | ||
| describe('performReachabilityAnalysis --use-only-socket-facts-artifacts gating', () => { |
There was a problem hiding this comment.
The four tests in this block have the same setup and call. They differ only in dynamicSbomInference, factsOnlyTypes and resolvedPathsSidecar, and in which flags must or must not appear. An it.for over { name, dynamicSbomInference, factsOnlyTypes, resolvedPathsSidecar, expected, absent } would cover them. The first test's check that the sidecar is empty at spawn time needs its own mockImplementationOnce, so it can stay a separate test.
Separately, coanaArgsFor (line 263) already wraps this call and the mock.calls[0]![0] read. Moving it to module scope and letting it take factsOnlyTypes would remove that boilerplate from these tests.
There was a problem hiding this comment.
[agent] Fixed in d283907 - coanaArgsFor is at module scope and takes the analysis options. The gating tests are one it.for table that also checks the sidecar contents at spawn time, which absorbed the empty-sidecar test.
| ) | ||
| }) | ||
|
|
||
| it('throws when no build root was discovered at all', async () => { |
There was a problem hiding this comment.
Several of these tests could be it.for tables:
- The three rejection tests (here, and at lines 128 and 161):
{ name, outcomes, error }. - The assertions on which options reach
generateRecursiveManifests(lines 30, 63 and 96): one row per consumer, withrequireDotnetSdk,sidecarAcc,withFilesandsbtTmpDir. The checks on the result's shape can stay as their own tests. - The two cases that don't throw (lines 177 and 210):
{ consumer, outcomes }, both asserting an emptyfactsPathsand no warning.
Separately, a run(consumer) helper, like generate() in the fix test, would remove the cwd, excludePaths and sbtTmpDir arguments repeated in every test.
There was a problem hiding this comment.
[agent] Fixed in d283907 - a run(consumer) helper, plus tables for the generator options per consumer, the rejections, and the quiet no-facts cases.
| }) | ||
|
|
||
| it('fails before building anything when a .NET build root needs an SDK that does not run', async () => { | ||
| const dir = await fs.mkdtemp(path.join(tmpdir(), 'dsi-require-sdk-')) |
There was a problem hiding this comment.
Nit: withTmpDir in src/utils/fs.mts does this mkdtemp plus try/finally cleanup. It's only worth switching for the whole file, since the rest of the file uses the same pattern.
There was a problem hiding this comment.
[agent] Not changing this here: as you say it's only worth it for the whole file, and the rest of the file is outside this PR.
…iring - --dynamic-sbom-inference and its setup wizard fail closed when no .NET SDK runs for a .NET build root, for every consumer, since no facts can be generated without it. --auto-manifest still warns and skips. - Name socket.json's path and the setup command in the next steps. - Write the reachability sidecar whenever facts-only types are set, so the flag and the sidecar `coana run` needs come from the same input. - Name the upload-only inference mode instead of an undefined consumer. - Move useOnlySocketFactsArtifactsFlags next to the other Coana helpers. - Table-driven tests with shared helpers.
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
There was a problem hiding this comment.
Looks good. I left a few comments: the reach fallback one is worth a look before merging, and the rest can be follow-ups.
| ): void { | ||
| const hasDisabled = outcomes.some(o => o.status === 'skippedDisabled') | ||
| // Coana (15.14.1) refuses a facts-only reachability run without any facts. | ||
| if (consumer === 'reach' && !hasFacts) { |
There was a problem hiding this comment.
This error, and the no-SDK one in generate-recursive-manifests.mts, now fire before reachability starts, so scan create --reach --reach-fallback-to-regular-scan can't fall back anymore. Previously Coana failed the facts-only run itself (buildSocketFactsSubprojectMap throws when the sidecar covers no facts file), reachResult.ok was false, and handle-create-new-scan.mts fell back to a regular scan. Now the whole scan create fails, and the flag the user passed is silently ignored. Could these go through the same fallback path when it's enabled?
| factsPaths: string[] | ||
| // Each discovered build root's purl type, generated or not: Coana takes | ||
| // these types' artifacts only from facts files. | ||
| factsOnlyTypes: PURL_Type[] |
There was a problem hiding this comment.
Nit: factsOnlyTypes sorts before factsPaths, here and in the object returned at line 118. The repo's CLAUDE.md asks for alphabetical order.
| ) { | ||
| outcomes.push({ ...root, ecosystem, status: 'skippedNoSdk' }) | ||
| continue | ||
| const reason = await noDotnetSdkReason(dirConfig.bin, dir) |
There was a problem hiding this comment.
Because this check runs per root inside the build loop, a later .NET root whose global.json pins a missing SDK aborts the run after earlier .NET roots were already built and wrote their facts files. fix resets those files, but scan create and scan reach leave them in the working tree, and the user waited for a build that can't be used. Checking every .NET root before building anything would make the "fails before building anything" test hold in general. Right now it holds only because the test has a single .NET root and dotnet comes first in BUILD_TOOLS.
| continue | ||
| const reason = await noDotnetSdkReason(dirConfig.bin, dir) | ||
| if (reason !== undefined) { | ||
| throw new InputError( |
There was a problem hiding this comment.
socket manifest dynamic-sbom-inference --json used to report a root without an SDK as a skippedNoSdk outcome in its JSON. Now this InputError escapes handleManifestDynamicSbomInference before its CResult is built, so a script parsing the JSON gets no outcome list.
| if (ecosystem === 'dotnet' && (await skipWithoutDotnetSdk(bin, dir))) { | ||
| return { ok: true, data: undefined } | ||
| if (ecosystem === 'dotnet') { | ||
| const reason = await noDotnetSdkReason(bin, dir) |
There was a problem hiding this comment.
The wizard checks for an SDK only on the path where nested roots are configured one by one. If the user answers no to "Configure other build roots found beneath this one, individually?" (line 853), it reports "Recursive setup complete." without checking any nested .NET root, and the next --dynamic-sbom-inference run fails. On the individual path, returning ok: false here looks like it aborts after earlier candidates' socket.json changes were written, leaving the setup half-applied.
| ok: false, | ||
| message: `No .NET SDK runs in ${path.relative(cwd, dir) || '.'} (${reason})`, | ||
| cause: | ||
| 'Install the .NET SDK, or point defaults.manifest.dotnet.bin at one; or leave the directory out with --exclude-paths.', |
There was a problem hiding this comment.
This remedy differs from the one in generate-recursive-manifests.mts: it omits the socket.json path and the defaults.manifest.dotnet.disabled option. A shared message builder next to noDotnetSdkReason would keep the two in sync.
| excludePaths, | ||
| sbtTmpDir: undefined, | ||
| }) | ||
| artifactsFlags = useOnlySocketFactsArtifactsFlags(factsOnlyTypes) |
There was a problem hiding this comment.
This repeats the sidecar and facts-only flag assembly from performReachabilityAnalysis, and the two copies already differ. Reach writes a sidecar whenever factsOnlyTypes is non-empty, but this writes one only when it has entries. Coana's fix commands don't require the sidecar today, so nothing breaks yet. Still, one helper returning the flags and the sidecar together would stop the two from drifting further.
|
|
||
| // Maven alone keeps the older flag, which Coana releases before 15.14.1 also | ||
| // know. | ||
| export function useOnlySocketFactsArtifactsFlags( |
There was a problem hiding this comment.
An empty types produces ['--use-only-socket-facts-artifacts', ''], which Coana rejects with "Unsupported PURL type(s)". performReachabilityAnalysis checks the length first, but generated-socket-facts.mts doesn't. A non-empty tuple type such as readonly [PURL_Type, ...PURL_Type[]] would rule this out at compile time, for example once a follow-up lets users pick ecosystems.
LLM Description written by Claude Code:claude-opus-5-5
Under
--dynamic-sbom-inference, reachability (scan create --reach,scan reach) andsocket fixtell Coana which ecosystems' artifacts come only from Socket facts files. Until now that was always Maven, plus NuGet only when some .NET build root happened to produce facts, so whether a raw manifest was trusted depended on what unrelated build roots produced.--maven-use-only-socket-factsflag, which Coana releases before 15.14.1 also know.--dynamic-sbom-inferenceand its setup wizard now fail closed, with next steps, when no .NET SDK runs for a .NET build root, instead of skipping it: the SDK is needed to generate its facts at all.--auto-manifeststays lenient and still warns and skips.socket fixleave them out.A follow-up (#1697) adds
--dynamic-sbom-inference-ecosystemsto choose which ecosystems use facts.Tested: unit tests for the new rule and checks; manual runs of the built CLI against a .NET project with no usable SDK (
manifest dynamic-sbom-inferencestops with next steps,manifest autowarns and skips).🤖 Generated with Claude Code