Repository navigation
feat(scan): add --dynamic-sbom-inference-ecosystems to choose which ecosystems use Socket facts - #1697
feat(scan): add --dynamic-sbom-inference-ecosystems to choose which ecosystems use Socket facts#1697Jeppe Fredsgaard Blaabjerg (jfblaa) wants to merge 3 commits into
Conversation
767c973 to
8a7fd77
Compare
…cosystems use Socket facts scan create, scan reach and fix take purl types (maven covers Gradle, sbt and Maven; nuget covers .NET). Build tools of unselected types are not discovered, so their manifest files are used as they are and Coana does not treat them as facts-only; with --auto-manifest, auto-generation still runs for them at cwd. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8a7fd77 to
31723a6
Compare
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
There was a problem hiding this comment.
Carried over from my review of #1696, since it merged with these open. Most of them are about code this PR doesn't touch, so they're below rather than inline.
The .NET SDK check runs per root inside the build loop (generate-recursive-manifests.mts). 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.
The no-SDK error skips the JSON output (same lines). socket manifest dynamic-sbom-inference --json used to report a root without an SDK as a skippedNoSdk outcome. Now the InputError escapes handleManifestDynamicSbomInference before its CResult is built, so a script parsing the JSON gets no outcome list.
The wizard checks for an SDK only on one path (setup-recursive-manifest-config.mts). If the user answers no to "Configure other build roots found beneath this one, individually?" (line 853), the wizard 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 looks like it aborts after earlier candidates' socket.json changes were written, leaving the setup half-applied.
Two versions of the no-SDK remedy (wizard and generation). The wizard's version 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.
useOnlySocketFactsArtifactsFlags accepts an empty list (coana.mts). An empty types produces ['--use-only-socket-facts-artifacts', ''], which Coana rejects with "Unsupported PURL type(s)". This PR can't produce an empty list, since cmdFlagValueToArray drops empty values and factsOnlyTypes comes from the discovered roots. A non-empty tuple type such as readonly [PURL_Type, ...PURL_Type[]] would still rule it out at compile time.
| }): 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, fire before reachability starts, so scan create --reach --reach-fallback-to-regular-scan can't fall back. When 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 is silently ignored. This PR makes it easier to hit: --dynamic-sbom-inference-ecosystems nuget on a Maven-only repo stops at the new "No build root for nuget was found" error before the fallback gets a chance. Could these errors go through the fallback path when it's enabled?
There was a problem hiding this comment.
[agent] Not changing this: --reach-fallback-to-regular-scan covers failures inside the Coana run, not --dynamic-sbom-inference preconditions. --dynamic-sbom-inference stays fail closed, as it has been for Maven, and falling back would upload the manifest files as they are, which is what fail closed rules out.
| }) | ||
| } | ||
| return { | ||
| factsPaths, |
There was a problem hiding this comment.
Nit: factsOnlyTypes sorts before factsPaths, here and in DynamicSbomInferenceResult. The repo's CLAUDE.md asks for alphabetical order.
There was a problem hiding this comment.
[agent] Fixed in 6748a9d - factsOnlyTypes now comes before factsPaths in DynamicSbomInferenceResult and the returned object.
| purlTypes, | ||
| 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.
There was a problem hiding this comment.
[agent] Fixed in 6748a9d - fix now writes the sidecar whenever it sends the facts-only flag, as reach does, so the two follow one rule. I tried a shared helper, but it pulled a command module's type into src/utils and mixed writing the file with building the flags, so each side keeps a one-line write. An empty sidecar is the same as none for the fixes call: Coana looks each facts file up in it.
Drop the dedicated .NET SDK probe from --dynamic-sbom-inference and its setup wizard: a missing SDK now fails the build root the way a missing Maven does, so the run aborts through the usual failed-root path and --json lists the root. --auto-manifest keeps warning and skipping. Also write the sidecar whenever the facts-only flag is sent, for fix as for reach, type the facts-only types as non-empty, and order the result fields.
|
[agent] On the points carried over from #1696 (all in 6748a9d):
|
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
There was a problem hiding this comment.
Simplification notes, nothing blocking. The most useful are the single dynamicSbomInference field, the shared flag helper and dropping narrowed: together they remove the extra field, the copied code in the three commands and the duplicated messages.
One more that is outside the diff: generate_auto_manifest.mts:229 still skips a .NET root without an SDK through skipWithoutDotnetSdk. The second commit's reasoning ("fail like any other build") would apply to --auto-manifest too, and skipWithoutDotnetSdk in scripts/run.mts could then be deleted. If the skip is intentional there, a short comment saying why would help.
|
|
||
| export type ReachabilityOptions = { | ||
| dynamicSbomInference: boolean | ||
| dynamicSbomInferenceEcosystems: readonly PURL_Type[] |
There was a problem hiding this comment.
dynamicSbomInference: boolean and dynamicSbomInferenceEcosystems together only ever mean "off" or "on with these ecosystems". Every reader of the list is already inside an if (…dynamicSbomInference) branch. A single dynamicSbomInference: FactsOnlyTypes | undefined (here and in fix/types.mts) would:
- make a selection with inference off impossible to represent;
- let
handle-ci.mts,create-scan-from-github.mtsandtest/reachability-options.mtsdrop theFACTS_PURL_TYPESthey import only to fill the field; - drop the extra field
handle-fixpasses through three places.
The existing if (x.dynamicSbomInference) checks keep working, because undefined is falsy.
| const dynamicSbomInferenceEcosystemsRaw = cmdFlagValueToArray( | ||
| cli.flags['dynamicSbomInferenceEcosystems'], | ||
| ) | ||
| const dynamicSbomInferenceEcosystems = parseDynamicSbomInferenceEcosystems( |
There was a problem hiding this comment.
This read-then-parse step, plus the "requires --dynamic-sbom-inference" entry in checkCommandInput below, is repeated in cmd-scan-reach.mts and cmd-fix.mts. Only this copy has a test. A helper in reachability-flags.mts could read both flags, throw InputError when ecosystems are given without --dynamic-sbom-inference (as the parser already does for an unknown value), and return the merged value from the ReachabilityOptions comment. Each command would then make one call.
One trade-off: the "requires" error would no longer use the checkCommandInput layout or exit code 2.
| verbose: false, | ||
| withFiles, | ||
| }) | ||
| const narrowed = purlTypes.length < FACTS_PURL_TYPES.length |
There was a problem hiding this comment.
narrowed only picks between two wordings of this error and of the one in checkFactsForCoana, which became an options object to carry it. Deriving the message from purlTypes would leave one version of each:
`No ${joinOr(buildToolsFor(purlTypes).map(t => ECOSYSTEM_LABELS[t]))} build root was found.`The hint lines can always offer both ways out. This would also name build tools instead of raw purl types, so it reads "No .NET build root" instead of "No build root for nuget". ECOSYSTEM_LABELS lives privately in setup-recursive-manifest-config.mts:49 and could move to build-tool.mts.
| type: 'string', | ||
| isMultiple: true, | ||
| description: | ||
| 'With --dynamic-sbom-inference: generate Socket facts only for these ecosystems, as either a comma separated value or as multiple flags; the others keep their manifest files as they are. Supported: maven (Gradle, sbt and Maven) and nuget (.NET). Defaults to both.', |
There was a problem hiding this comment.
The supported list is written out by hand here, so a new entry in PURL_TYPE_BY_TOOL would be accepted by the parser but missing from the help text. It could be built from FACTS_PURL_TYPES (plus labels), the way --reach-ecosystems builds its list from getReachabilityEcosystemChoices().
| export function parseEcosystemChoices( | ||
| raw: readonly string[], | ||
| choices: readonly string[], | ||
| ): PURL_Type[] { |
There was a problem hiding this comment.
Lowercasing and de-duplication happen only in parseDynamicSbomInferenceEcosystems, so three ecosystem-list flags now behave differently:
--dynamic-sbom-inference-ecosystemsignores case and removes duplicates.--reach-ecosystemsis case-sensitive and keeps duplicates.fix --ecosystemsignores case through its own loop atcmd-fix.mts:422, and reports errors withlogger.failinstead of throwing.
Moving both steps in here would make the three consistent and let fix --ecosystems use this parser too. Nit: arrayUnique from @socketsecurity/registry/lib/arrays is already used elsewhere in place of [...new Set(…)].
|
|
||
| export type BuildTool = 'dotnet' | 'gradle' | 'maven' | 'sbt' | ||
|
|
||
| export const BUILD_TOOLS: readonly BuildTool[] = [ |
There was a problem hiding this comment.
Minor: PURL_TYPE_BY_TOOL below is an exhaustive record of the same keys, so this could be Object.keys(PURL_TYPE_BY_TOOL) as BuildTool[]. A new tool would then need adding in one place instead of two.
| await expect(run('reach')).rejects.toThrow(error) | ||
| }) | ||
|
|
||
| it.for([ |
There was a problem hiding this comment.
This table and the one above have the same mocks and assertion, so they could be one it.for table with columns { name, outcomes, purlTypes?, error }, calling run('reach', purlTypes). The purlTypes: ['nuget'] forwarding check repeats what "generates for $consumer" already checks.
| expect(candidates.get('gradle')?.length).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| it('drops the build tools of unselected purl types', async () => { |
There was a problem hiding this comment.
This test runs two scenarios with different assertion styles. As it.for([{ purlTypes: ['maven'], tools: ['gradle', 'maven', 'sbt'] }, { purlTypes: ['nuget'], tools: ['dotnet'] }]) asserting expect([...c.keys()]).toEqual(tools), each scenario reports separately. "drops a disabled ecosystem entirely" above could join as a row with a sockJson column.
| }, | ||
| ) | ||
|
|
||
| cmdit( |
There was a problem hiding this comment.
This and the test above differ only in the extra flags and the expected stderr, so they could be a { args, error } table on a shared base-argument list, like the uvBaseArgs table at the top of the file. If the flag parsing moves into a shared helper (see the cmd-scan-create.mts comment), a plain it.for on that helper would be better still. It would cover all three commands without spawning the CLI, and the parser has no unit test today.
| }) | ||
| }) | ||
|
|
||
| it('writes an empty sidecar when no build root resolved any paths', async () => { |
There was a problem hiding this comment.
Optional: this and "writes the sidecar outside the repository" could be a table with columns { factsOnlyTypes, resolvedPathsSidecar, artifactsFlags, sidecar }. The first test also checks the forwarded arguments and the sidecar's directory, though, so with two rows the table saves little.
…ield - One `dynamicSbomInference` option holds the selected ecosystems, or undefined when off, in place of a boolean plus a list. - `parseDynamicSbomInferenceFlags` reads both flags for scan create, scan reach and fix. - Ecosystem lists are parsed case-insensitively and de-duplicated by one parser, which `fix --ecosystems` now uses too. - The "no build root" message names the selected build tools, and the flag's help lists the supported ecosystems from the build-tool table. - `--auto-manifest` fails a .NET build without a usable SDK, as it does a Maven build without Maven, instead of skipping it.
LLM Description written by Claude Code:claude-opus-5-5
Adds
--dynamic-sbom-inference-ecosystemstosocket scan create,socket scan reachandsocket fix, so a repo can use Socket facts for some ecosystems and keep the manifest files of others as they are.maven(Gradle, sbt and Maven) andnuget(.NET). Without the flag, both are used, as before.--auto-manifest, auto-generation still runs for them at the scan root.--dynamic-sbom-inference, accepts any case, and rejects other ecosystems. Errors for "no build root found" name the selected build tools, and both that error and "no facts produced" suggest choosing other ecosystems.dynamicSbomInferenceoption carries the selection (undefined when off), read by one parser shared by the three commands.--reach-ecosystemsandfix --ecosystemsnow also accept any case, through the same ecosystem parser.Follow-ups to the #1696 review:
--jsonlists it as a failed root.--auto-manifestandsocket manifest autonow fail the same way instead of skipping the build.socket fixwrites the sidecar whenever it sends the facts-only flag, as reach does, and the facts-only types are typed non-empty.Tested: unit tests for discovery filtering, the selection passed through scan create, scan reach and fix, the auto-manifest interplay, and flag parsing; manual runs of the built CLI on a Maven + .NET directory (
mavenselected generates Maven facts and leaves .NET alone;nugetselected on a Maven-only dir fails with the narrowed message).🤖 Generated with Claude Code