Skip to content

Commit 71a2a3e

Browse files
committed
review(#1318): address PR feedback (findings 1, 2, 3, 4, 6, 8)
Finding 1 — Kotlin Multiplatform fixture: bump `org.jetbrains.kotlin.multiplatform` from 1.9.25 (officially supports Gradle 6.8.3-8.6) to 2.1.0 (supports Gradle 9.x). CI installs Gradle 9.2.1 so the old plugin version would fail. Smoke-tested locally. Finding 2 — `handle-create-new-scan.mts`: rename `filterToCdxSpdxAndFactsFiles` → `filterToCdxSpdxOnly`. The "AndFactsFiles" branch in the function was dead at the only call site because the caller had already stripped `.socket.facts.json` from the input. Dropped the basename check from the function body; semantics unchanged. Finding 3 — Aggregator's `node.children` read now happens under `synchronized(nodes)` to mirror the writers in each `socketFactsCollect` doLast. Task dependencies via `aggregator.dependsOn(collector)` already establish a happens-before edge, but explicit synchronization makes the contract local and removes any implicit reliance on Gradle's task-graph memory visibility semantics. The aggregator snapshots into plain List/Map values first, then builds the JSON outside the critical section. Finding 4 — `convert-gradle-to-facts.mts`: replace `output.stdout.replace(regex, callback)`-for-side-effects pattern with `Array.from(stdout.matchAll(regex), m => m[1])`. Reads more directly. Finding 6 — Cross-reference comments at both `ext.SOCKET_FACTS_FILENAME` (Groovy) and `DOT_SOCKET_DOT_FACTS_JSON` (TS) noting that the two values are intentionally duplicated (Groovy can't import a TS constant) and must be kept in sync. Finding 8 — When the Gradle script's `components.isEmpty()` branch fires (no resolvable dependencies in the build), the TS wrapper no longer prints a bare "Reported exports:" header followed by nothing. It now suppresses the header when `matchAll` returns zero exports, and surfaces the `[socket-facts] no resolvable dependencies` skip message from Gradle stdout if present so the user understands why the file wasn't written. Not changed (replies posted separately): - Finding 5: `.toLowerCase()` IS load-bearing on case-insensitive filesystems (macOS HFS+, Windows). The constant is lowercase; the input might not be. Keeping the normalization. - Finding 7: function ordering left to match the existing `convert_gradle_to_maven.mts` pattern — consistency with precedent wins over the literal CLAUDE.md rule here.
1 parent d6a49f7 commit 71a2a3e

5 files changed

Lines changed: 57 additions & 25 deletions

File tree

src/commands/manifest/convert-gradle-to-facts.mts

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,27 @@ export async function convertGradleToFacts({
7474
)
7575
return
7676
}
77-
logger.log('Reported exports:')
78-
output.stdout.replace(
79-
/^Socket facts file written to: (.*)/gm,
80-
(_all: string, fn: string) => {
81-
logger.log('- ', fn)
82-
return fn
83-
},
77+
const exports = Array.from(
78+
output.stdout.matchAll(/^Socket facts file written to: (.*)/gm),
79+
m => m[1],
8480
)
81+
if (exports.length) {
82+
logger.log('Reported exports:')
83+
for (const fn of exports) {
84+
logger.log('- ', fn)
85+
}
86+
} else {
87+
// Gradle script may have skipped emission when no resolvable
88+
// dependencies were found (see the `components.isEmpty()` branch in
89+
// socket-facts.init.gradle). Surface the skip reason if present so
90+
// the user understands why nothing was written.
91+
const skipMatch = output.stdout.match(
92+
/^\[socket-facts\] no resolvable dependencies.*/m,
93+
)
94+
if (skipMatch) {
95+
logger.warn(skipMatch[0])
96+
}
97+
}
8598
logger.log('')
8699
logger.log(
87100
'Next step is to generate a Scan by running the `socket scan create` command on the same directory.',

src/commands/manifest/socket-facts.init.gradle

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@
3333
import java.util.Collections
3434
import groovy.json.JsonOutput
3535

36+
// Must stay in sync with `DOT_SOCKET_DOT_FACTS_JSON` in
37+
// src/constants.mts (TS side). Groovy can't import the TS constant, so
38+
// the two strings are intentionally duplicated; if you change one,
39+
// change the other.
3640
ext.SOCKET_FACTS_FILENAME = '.socket.facts.json'
3741

3842
// Shared accumulators across all subprojects' contributions. Synchronized
@@ -266,8 +270,23 @@ rootProject {
266270
def nodes = state.nodes
267271
def directIds = state.directIds
268272

269-
def components = nodes.collect { id, node ->
270-
def coord = node.coord
273+
// Snapshot the accumulators under the same monitor used by writers in
274+
// each subproject's socketFactsCollect doLast. Task dependencies
275+
// (`aggregator.dependsOn(collector)`) already guarantee a
276+
// happens-before edge between writes and this read, but we
277+
// synchronize on `nodes` here so the read path is symmetric with the
278+
// write path — no implicit reliance on Gradle's task-graph ordering
279+
// semantics for memory visibility of plain HashMap/HashSet fields.
280+
def components
281+
synchronized (nodes) {
282+
components = nodes.collect { id, node ->
283+
[id: id, coord: node.coord, prod: node.prod, nonTooling: node.nonTooling, children: (node.children as List).sort()]
284+
}
285+
}
286+
287+
components = components.collect { snapshot ->
288+
def id = snapshot.id
289+
def coord = snapshot.coord
271290
def component = [
272291
type : 'maven',
273292
namespace: coord.groupId,
@@ -290,14 +309,14 @@ rootProject {
290309
if (directIds.contains(id)) {
291310
component.direct = true
292311
}
293-
if (!node.prod) {
312+
if (!snapshot.prod) {
294313
component.dev = true
295314
}
296-
if (!node.nonTooling) {
315+
if (!snapshot.nonTooling) {
297316
component.tooling = true
298317
}
299-
if (!node.children.isEmpty()) {
300-
component.dependencies = (node.children as List).sort()
318+
if (!snapshot.children.isEmpty()) {
319+
component.dependencies = snapshot.children
301320
}
302321
component
303322
}

src/commands/scan/handle-create-new-scan.mts

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,20 +47,14 @@ function getCdxSpdxPatterns(
4747
return patterns
4848
}
4949

50-
function filterToCdxSpdxAndFactsFiles(
50+
function filterToCdxSpdxOnly(
5151
filepaths: string[],
5252
supportedFiles: SocketSdkSuccessResult<'getReportSupportedFiles'>['data'],
5353
): string[] {
5454
const patterns = getCdxSpdxPatterns(supportedFiles)
55-
return filepaths.filter(filepath => {
56-
const basename = path.basename(filepath).toLowerCase()
57-
// Include .socket.facts.json files.
58-
if (basename === constants.DOT_SOCKET_DOT_FACTS_JSON) {
59-
return true
60-
}
61-
// Include CDX and SPDX files.
62-
return micromatch.some(filepath, patterns, { nocase: true })
63-
})
55+
return filepaths.filter(filepath =>
56+
micromatch.some(filepath, patterns, { nocase: true }),
57+
)
6458
}
6559

6660
export type HandleCreateNewScanConfig = {
@@ -270,7 +264,7 @@ export async function handleCreateNewScan({
270264

271265
// When using pregenerated SBOMs only, filter to CDX/SPDX files.
272266
const pathsForScan = reach.reachUseOnlyPregeneratedSboms
273-
? filterToCdxSpdxAndFactsFiles(filteredPackagePaths, supportedFiles)
267+
? filterToCdxSpdxOnly(filteredPackagePaths, supportedFiles)
274268
: filteredPackagePaths
275269

276270
scanPaths = [

src/constants.mts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,10 @@ const CONFIG_KEY_API_TOKEN = 'apiToken'
199199
const CONFIG_KEY_DEFAULT_ORG = 'defaultOrg'
200200
const CONFIG_KEY_ENFORCED_ORGS = 'enforcedOrgs'
201201
const CONFIG_KEY_ORG = 'org'
202+
// Must stay in sync with `ext.SOCKET_FACTS_FILENAME` in
203+
// src/commands/manifest/socket-facts.init.gradle (Groovy side).
204+
// Groovy can't import a TS constant, so the two values are intentionally
205+
// duplicated; change them together.
202206
const DOT_SOCKET_DOT_FACTS_JSON = `${DOT_SOCKET_DIR}.facts.json`
203207
const DLX_BINARY_CACHE_TTL = 7 * 24 * 60 * 60 * 1_000 // 7 days in milliseconds.
204208
const DRY_RUN_LABEL = '[DryRun]'

test/fixtures/commands/manifest/gradle-facts/kotlin-multiplatform/build.gradle

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@
33
// jsTestRuntimeClasspath, etc.) that the Java SourceSetContainer doesn't
44
// surface, but which our name-pattern selection (`*CompileClasspath` /
55
// `*RuntimeClasspath`) still picks up.
6+
// Kotlin 2.1.x is required for Gradle 9.x compatibility. KGP 1.9.x
7+
// officially supports Gradle 6.8.3-8.6 only; CI runs Gradle 9.2.1.
68
plugins {
7-
id 'org.jetbrains.kotlin.multiplatform' version '1.9.25'
9+
id 'org.jetbrains.kotlin.multiplatform' version '2.1.0'
810
}
911

1012
group = 'com.example.socket.kmp'

0 commit comments

Comments
 (0)