Skip to content

Commit 0e41460

Browse files
authored
fix(manifest/bazel): harden Maven extraction completeness and show_extension handling (#1364)
* fix(manifest/bazel): pin lockfile read-only for show_extension scans The mod show_extension argv builders for the Maven and pip extensions did not pass --lockfile_mode=off, so a read-only dependency scan could rewrite the user's MODULE.bazel.lock. Mirror the query/cquery builders and pin the lockfile read-only before user flags. * fix(manifest/bazel): detect cquery timeouts by the kill shape spawn emits The cquery timeout detection tested for a `timedOut` flag the registry spawn never sets; on a timeout the child is killed and Node reports `killed: true` with `signal: SIGTERM` (or SIGKILL). Drop the phantom `timedOut` branch and update the test to assert the real kill shape. * fix(manifest/bazel): surface incomplete Maven extraction honestly A Bazel Maven extraction that under-reports dependencies must never be presented downstream as a complete SBOM. This reworks the shared status model so the CLI is honest about partial and unanalyzable runs: - Add an `indeterminate` probe state. An unrecognized non-zero probe exit or a thrown probe is no longer swallowed as `not-defined` ("no Maven here"); it is propagated so the run is never reported complete. - A workspace that fails to load is recorded as a load failure rather than silently skipped: a hard failure when nothing else was analyzable, a partial otherwise. - Enrich the extraction result with a machine-readable completeness signal (a `complete` flag plus per-workspace / per-hub outcomes) and write a completeness summary alongside the manifests for downstream consumers. The partial SBOM is still emitted and the warning stays loud. - Make both the explicit command and auto-manifest honest: exit non-zero only on hard failure, exit 0 on partial with a prominent warning and the completeness signal echoed. - Gate synthetic hub manifests on committed lockfiles: when a committed maven_install.json / <hub>_maven_install.json already covers a hub, skip re-emitting it since the server already ingests committed lockfiles. - Wire a --per-repo-timeout flag (and socket.json setting) with a 120s default for the explicit command, longer than the 60s auto-manifest default; drop the misleading comment claiming the default already existed. Per-state diagnostics are logged under --verbose. * test(manifest/bazel): index access instead of Array.at for mock call lookup Array.prototype.at requires the es2022 lib; the stricter typechecker flagged it on the mock.calls tuple type. Use length-1 index access, which is lib-agnostic. * fix(manifest/bazel): scope committed-lockfile gate to its own workspace root The committed-lockfile gate walked the whole tree under a workspace root and matched a maven_install.json at any depth. A nested workspace or test fixture lockfile then wrongly marked the root @maven hub as already covered, so its synthetic manifest was skipped and its distinct coordinates were never emitted while the run could still report complete. Scope the gate to depth-0: a committed lockfile only covers the workspace it lives directly in, since each workspace is analyzed independently and the server walker ingests every committed lockfile against its own workspace. Also exclude the CLI's own synthetic output directories (.socket-auto-manifest, bazel-manifests) by name so a stale prior-run manifest can never be misread as a committed lockfile. Mark Maven hub enumeration indeterminate when `bazel mod show_extension` fails to execute (non-zero exit), distinct from a clean run that finds no maven extension. A failed enumeration may have missed custom-named hubs, so the run is reported known-incomplete (partial when other hubs succeeded, hard failure when nothing is analyzable) instead of silently falling back to conventional-name probes and reporting complete. * fix(manifest/bazel): classify show_extension non-zero exits instead of blanket-flagging bazel mod show_extension for the maven extension exits non-zero on every bzlmod repo that doesn't depend on rules_jvm_external, because its argument resolution throws before any Starlark runs. Treating any non-zero exit as an indeterminate enumeration wrongly flipped those legitimate no-Maven repos to a hard failure, aborting the user's whole auto-manifest scan. Classify the result by stderr shape via classifyShowExtensionResult: - extension/module not in the dependency graph (arg-resolution error) is a legitimate not-defined, contributing no Maven (noEcosystem) - a genuine MODULE.bazel evaluation/load failure, or a missing binary (normalized code -1), stays indeterminate so the run is never complete - an unrecognized non-zero exit biases to not-defined; only positive eval-failure stderr escalates to indeterminate Replace the old hard-fail test (whose mock was a legitimate no-Maven repo) with a noEcosystem assertion, and add coverage for the not-in-graph and genuine-eval-failure paths. * fix(manifest/bazel): match real show_extension not-in-graph wording The not-in-graph classifier relied on the conservative default for Bazel's actual phrasing ('No module with the apparent repo name X exists in the dependency graph'). Add that verified wording explicitly and pin both the real not-in-graph (exit 2 -> not-defined) and real unbound-name eval failure (exit 2 -> indeterminate, eval-failure checked first) with tests.
1 parent 2443ac7 commit 0e41460

13 files changed

Lines changed: 1580 additions & 76 deletions

src/commands/manifest/bazel/bazel-cquery.mts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -551,12 +551,13 @@ export async function runMetadataCqueryForRepo(
551551
signal?: unknown
552552
stderr?: unknown
553553
stdout?: unknown
554-
timedOut?: unknown
555554
}
556555
const stdout = typeof err.stdout === 'string' ? err.stdout : ''
557556
const stderr = typeof err.stderr === 'string' ? err.stderr : ''
557+
// On a `timeout`, the registry spawn kills the child, so Node sets
558+
// `killed: true` and `signal: 'SIGTERM'` (or `SIGKILL`). There is no
559+
// `timedOut` flag on the real rejection, so do not test for one.
558560
const timedOut =
559-
err.timedOut === true ||
560561
err.killed === true ||
561562
err.signal === 'SIGTERM' ||
562563
err.signal === 'SIGKILL'

src/commands/manifest/bazel/bazel-cquery.test.mts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -642,11 +642,15 @@ describe('runMetadataCqueryForRepo', () => {
642642
expect(r.artifacts).toEqual([])
643643
})
644644

645-
it('returns status:timeout when spawn rejects with timedOut=true', async () => {
645+
it('returns status:timeout when spawn is killed on timeout (killed=true + SIGTERM)', async () => {
646+
// The real registry spawn does not set `timedOut`; on a `timeout` it kills
647+
// the child, so Node populates `killed: true` and `signal: 'SIGTERM'`.
648+
// Mock that shape so the test pins the behaviour real spawn produces.
646649
mocked.mockRejectedValueOnce(
647650
Object.assign(new Error('command timed out'), {
648651
code: null,
649-
timedOut: true,
652+
killed: true,
653+
signal: 'SIGTERM',
650654
stderr: '',
651655
stdout: '',
652656
}),

src/commands/manifest/bazel/bazel-query-runner.mts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,10 @@ function buildBazelModShowMavenExtensionArgv(
8585
'mod',
8686
'show_extension',
8787
'@rules_jvm_external//:extensions.bzl%maven',
88+
// A read-only scan must never rewrite the user's MODULE.bazel.lock; pin
89+
// the lockfile read-only before user flags, mirroring the query/cquery
90+
// argv builders.
91+
'--lockfile_mode=off',
8892
// Belt-and-suspenders output reducer mirroring the PyPI path: bias the
8993
// report toward the root module's usages. The authoritative pruning is
9094
// the importers-filter applied to the parsed output, so this is not
@@ -101,6 +105,10 @@ function buildBazelModShowPipExtensionArgv(opts: BazelQueryOptions): string[] {
101105
'mod',
102106
'show_extension',
103107
'@rules_python//python/extensions:pip.bzl%pip',
108+
// A read-only scan must never rewrite the user's MODULE.bazel.lock; pin
109+
// the lockfile read-only before user flags, mirroring the query/cquery
110+
// argv builders.
111+
'--lockfile_mode=off',
104112
'--extension_usages=<root>',
105113
...userFlags,
106114
]

src/commands/manifest/bazel/bazel-query-runner.test.mts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,10 +256,21 @@ describe('runBazelModShowMavenExtension', () => {
256256
'mod',
257257
'show_extension',
258258
'@rules_jvm_external//:extensions.bzl%maven',
259+
'--lockfile_mode=off',
259260
'--extension_usages=<root>',
260261
])
261262
})
262263

264+
it('pins the lockfile read-only so the scan never rewrites MODULE.bazel.lock', async () => {
265+
await runBazelModShowMavenExtension({
266+
bin: 'bazel',
267+
cwd: '/repo',
268+
invocationFlags: [],
269+
})
270+
const argv = mocked.mock.calls[0]![1] as string[]
271+
expect(argv).toContain('--lockfile_mode=off')
272+
})
273+
263274
it('threads outputUserRoot ahead of the subcommand', async () => {
264275
await runBazelModShowMavenExtension({
265276
bin: 'bazel',
@@ -273,6 +284,7 @@ describe('runBazelModShowMavenExtension', () => {
273284
'mod',
274285
'show_extension',
275286
'@rules_jvm_external//:extensions.bzl%maven',
287+
'--lockfile_mode=off',
276288
'--extension_usages=<root>',
277289
])
278290
})
@@ -320,6 +332,7 @@ describe('runBazelModShowPipExtension', () => {
320332
'mod',
321333
'show_extension',
322334
'@rules_python//python/extensions:pip.bzl%pip',
335+
'--lockfile_mode=off',
323336
'--extension_usages=<root>',
324337
])
325338
})

src/commands/manifest/bazel/bazel-repo-discovery.mts

Lines changed: 127 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,17 @@ export type ProbeResult = {
3636

3737
export type RepoProbe = (repoName: string) => Promise<ProbeResult>
3838

39-
export type ProbeStatus = 'populated' | 'empty' | 'not-defined'
39+
// `indeterminate` means the probe could not be classified: an unrecognized
40+
// non-zero exit, or the probe threw outright (the Bazel invocation itself
41+
// failed). It is NOT evidence that the repo is undefined — treating it as
42+
// `not-defined` would silently under-report a hub that may well hold Maven
43+
// deps. The orchestrator must propagate it so the run is never reported
44+
// `complete` when a probe was indeterminate.
45+
export type ProbeStatus =
46+
| 'populated'
47+
| 'empty'
48+
| 'not-defined'
49+
| 'indeterminate'
4050

4151
// Conventional Maven hub names rules_jvm_external sets up under
4252
// WORKSPACE-mode invocations. Probing each one is cheap (a failed visibility
@@ -76,6 +86,109 @@ const SHOW_EXT_SECTION_HEADER_RE =
7686
const FETCHED_HUB_BULLET_RE =
7787
/^ {2}- (?<name>\S+) \(imported by (?<importers>[^)]+)\)\s*$/
7888

89+
// `bazel mod show_extension @rules_jvm_external//:extensions.bzl%maven`
90+
// exits non-zero in two very different situations, and conflating them is
91+
// dangerous for a security tool:
92+
//
93+
// (a) `@rules_jvm_external` simply isn't in the root module's resolved
94+
// dependency graph. This is the COMMON case for any bzlmod repo that
95+
// doesn't use rules_jvm_external (no Maven at all). Bazel's ModCommand
96+
// resolves the extension argument up front via
97+
// `ExtensionArg.resolveToExtensionId`, which throws
98+
// `InvalidArgumentException` and exits non-zero before evaluating any
99+
// Starlark. This is NOT a failure to analyze; it is a positive,
100+
// authoritative "there is no maven extension here". It must map to
101+
// `not-defined` so the workspace cleanly contributes no Maven.
102+
//
103+
// (b) The module graph genuinely fails to evaluate: a Starlark eval error,
104+
// an unbound name (e.g. a MODULE.bazel referencing `PYTHON_VERSION` /
105+
// `pip` before definition), a syntax error, or the bazel binary itself
106+
// being missing/spawn-failed (normalized to code -1). Here we have NO
107+
// evidence about whether a maven extension exists, so it must map to
108+
// `indeterminate` and the run can never be reported complete.
109+
//
110+
// We classify by stderr shape. The exact wording differs across Bazel
111+
// versions; the regex families below are intentionally broad and SHOULD be
112+
// confirmed against live `bazel mod show_extension` output.
113+
114+
// Family (a): the extension / module is not resolvable in the dependency
115+
// graph — an argument-resolution error, not an evaluation failure. These all
116+
// mean "rules_jvm_external (and thus the maven extension) is not present",
117+
// i.e. legitimately not-defined. The `no module ... exists in the dependency
118+
// graph` branch is Bazel's verified real wording (`bazel mod show_extension`
119+
// against a bzlmod repo without rules_jvm_external: "No module with the
120+
// apparent repo name @rules_jvm_external exists in the dependency graph").
121+
const SHOW_EXT_NOT_IN_GRAPH_STDERR_RE =
122+
/(?:in extension argument|extension argument)?.*(?:not (?:found|resolvable|defined)|no such (?:module|repo(?:sitory)?)|cannot be resolved|is not (?:a )?(?:visible |known )?(?:module|repo(?:sitory)?|extension)|not in the (?:dependency )?graph|no module[^\n]*exists in the (?:dependency )?graph|unknown (?:module|extension)|does not (?:exist|use the extension))/i
123+
// Bazel's canonical phrasing when the named module backing the extension
124+
// (here `rules_jvm_external`) isn't a dependency of the root module.
125+
const SHOW_EXT_MODULE_NOT_DEP_STDERR_RE =
126+
/(?:rules_jvm_external|module ['"`]?[A-Za-z0-9._+~-]+['"`]?).*(?:is not (?:a )?(?:direct )?dep(?:endenc(?:y|ies))?|not (?:a )?dependency)/i
127+
128+
// Family (b): a genuine evaluation / load failure of the module graph. These
129+
// mean we could not determine whether a maven extension exists, so the result
130+
// is indeterminate, never a clean not-defined.
131+
const SHOW_EXT_EVAL_FAILURE_STDERR_RE =
132+
/(?:error (?:evaluating|loading|computing)|failed to (?:evaluate|load)|evaluation (?:of|failed)|cannot load|syntax error|name ['"`]?[A-Za-z0-9_]+['"`]? is not defined|variable ['"`]?[A-Za-z0-9_]+['"`]? (?:is|was) (?:referenced|not)|unbound|invalid MODULE\.bazel|MODULE\.bazel.*(?:error|failed)|Traceback|Error in)/i
133+
134+
// Outcome of running `bazel mod show_extension` for the maven extension,
135+
// distinct from the per-repo `ProbeStatus`:
136+
// `not-defined` — authoritative: no maven extension in this workspace
137+
// (clean run with zero kept hubs, OR rules_jvm_external is
138+
// not in the dependency graph).
139+
// `indeterminate` — enumeration could not be performed (eval/load failure,
140+
// binary missing); the run must not be reported complete.
141+
// `defined` — the report parsed and yielded one or more root hubs;
142+
// the caller uses the parsed hub list directly.
143+
export type ShowExtensionStatus = 'defined' | 'indeterminate' | 'not-defined'
144+
145+
// Classify a `bazel mod show_extension` result. `keptRootHubCount` is the
146+
// number of root-imported hubs the caller parsed from a code-0 run (see
147+
// `parseShowExtensionOutput` + the `<root>` importer filter); it disambiguates
148+
// the code-0 cases without re-parsing here.
149+
//
150+
// IMPORTANT (security correctness): a non-zero exit is the DEFAULT outcome for
151+
// every bzlmod repo that does not use rules_jvm_external, so we must NOT treat
152+
// non-zero as indeterminate by default. We only escalate to `indeterminate`
153+
// when stderr looks like a real evaluation/load failure; an argument/resolution
154+
// error about the missing extension is the legitimate no-Maven case.
155+
export function classifyShowExtensionResult(
156+
result: ProbeResult,
157+
keptRootHubCount: number,
158+
): ShowExtensionStatus {
159+
if (result.code === 0) {
160+
// Clean run. Either it enumerated root hubs (`defined`) or it ran fine and
161+
// found no maven extension for the root (`not-defined`).
162+
return keptRootHubCount > 0 ? 'defined' : 'not-defined'
163+
}
164+
// A spawn failure / missing binary is normalized to code -1 upstream; there
165+
// is no usable stderr classification and we definitely could not enumerate.
166+
if (result.code === -1) {
167+
return 'indeterminate'
168+
}
169+
const { stderr } = result
170+
// A genuine module-graph evaluation/load failure wins: we cannot conclude
171+
// anything about maven presence, so surface it as indeterminate.
172+
if (SHOW_EXT_EVAL_FAILURE_STDERR_RE.test(stderr)) {
173+
return 'indeterminate'
174+
}
175+
// The maven extension / rules_jvm_external is simply not in the dependency
176+
// graph: an argument-resolution error. This is the common no-Maven bzlmod
177+
// repo and is authoritatively not-defined.
178+
if (
179+
SHOW_EXT_NOT_IN_GRAPH_STDERR_RE.test(stderr) ||
180+
SHOW_EXT_MODULE_NOT_DEP_STDERR_RE.test(stderr)
181+
) {
182+
return 'not-defined'
183+
}
184+
// Truly unrecognized non-zero exit. Bias toward not-defined: the dominant
185+
// real-world non-zero case is "extension not in the graph", and a missing
186+
// bullet here would otherwise abort the user's entire scan. We only reach
187+
// `indeterminate` above when stderr positively looks like an eval/load
188+
// failure, which is the case the flag exists for.
189+
return 'not-defined'
190+
}
191+
79192
// Pure parser for `bazel mod show_extension @rules_jvm_external//:extensions.bzl%maven`
80193
// stdout. Returns the hub repos listed under `Fetched repositories:` — i.e.
81194
// items annotated with `(imported by ...)` — each carrying the set of modules
@@ -154,13 +267,16 @@ export function classifyProbeResult(result: ProbeResult): ProbeStatus {
154267
return 'empty'
155268
}
156269
// Code 0 with empty stdout: WORKSPACE-mode probes do this when the repo
157-
// name isn't declared (Exp 5c). Treat as not-defined.
270+
// name isn't declared. Treat as not-defined.
158271
if (result.code === 0) {
159272
return 'not-defined'
160273
}
161-
// Code 1 with no recognizable message: be conservative and call it
162-
// not-defined so the orchestrator skips it without erroring the workspace.
163-
return 'not-defined'
274+
// Non-zero exit with no recognizable message: the probe failed for a reason
275+
// we can't classify (Bazel infra error, analysis crash, unexpected stderr).
276+
// This is NOT proof the repo is undefined, so do NOT downgrade it to
277+
// not-defined — surface it as indeterminate so the orchestrator can flag
278+
// the workspace as not fully analyzable rather than silently skipping it.
279+
return 'indeterminate'
164280
}
165281

166282
// Convenience: probe a single candidate and return its classified status,
@@ -176,14 +292,18 @@ export async function probeCandidate(
176292
try {
177293
result = await probe(repoName)
178294
} catch (e) {
295+
// A thrown probe means the Bazel invocation itself failed; we have no
296+
// evidence about whether the repo exists. Surface it as indeterminate so
297+
// the run is not reported complete, rather than swallowing it as a
298+
// not-defined skip.
179299
if (verbose) {
180300
logger.log(
181-
`[VERBOSE] discovery: probe @${repoName}: not-defined (probe threw: ${
301+
`[VERBOSE] discovery: probe @${repoName}: indeterminate (probe threw: ${
182302
e instanceof Error ? e.message : String(e)
183303
})`,
184304
)
185305
}
186-
return 'not-defined'
306+
return 'indeterminate'
187307
}
188308
const status = classifyProbeResult(result)
189309
if (verbose) {

0 commit comments

Comments
 (0)