commands/sbom: compose per-runtime and per-extension SBOMs by hiagofranco · Pull Request #238 · avocado-linux/avocado-cli

hiagofranco · 2026-09-04T14:33:53Z

Summary

ENG-2405 emitted one software_Sbom covering every sysroot at once, so no
runtime or extension had an artifact of its own. This adds one per runtime,
per extension, and — with --include-sdk — one for the build host, and
attaches the runtime's own document to connect upload.

More software_Sbom elements in the one graph, not one file per extension.
A package shared between rootfs and a runtime stays one element with two
contains edges; split across files it becomes two, and a scanner counting
packages or matching CVEs counts the same exposure twice. A standalone
extension file also could not name its parent runtime without SPDX imports
machinery nothing here emits.
build_document was already a pure function of the scope slice handed to
it, so connect upload builds a runtime-scoped document by filtering
rather than through any new CLI surface.
The attach never fails an upload: a build error warns and sends nothing,
the sbom field is skipped when absent (so a server that doesn't read it
yet, ENG-2284, sees today's request unchanged), and a 4xx retries once
without it — likeliest cause is 413, since ~400 packages of SPDX is
megabytes into a body that's otherwise kilobytes. AVOCADO_UPLOAD_NO_SBOM=1
skips the build entirely.
An includes:<n> scope is a remote extension, not a project include, so it
gets its own document too.
The device-level SBOM stays first in the graph (existing tests take the
first software_Sbom match) and now states in prose, not only via
software_sbomType, what it describes.

Closes ENG-2219.

Test plan

cargo test green (1517+9+2+6+8+135 passed; the one clean test
failure locally is an unrelated podman volume-cleanup race, not
touched by this change)
Verified push order: device SBOM remains the first software_Sbom
element in the graph

jetm

Read this with the artifact's consumer in mind rather than the code alone: an SBOM that is quietly incomplete is worse than one that fails to build, because nothing downstream can tell the difference. Four inline comments, and three of them are that shape - a document that looks whole and is not.

The composition mechanics themselves hold up. A package in two extensions collapses to one entry, conflicting versions stay two distinct elements, the slug-collision path is covered by the SHA-256 suffix, and is_client_error survives two .context() layers - I compiled a probe for that last one rather than reading it off the types.

cargo test --all-targets passes, 3277 tests across 15 suites, and cargo fmt --check is clean. cargo clippy -D warnings exits 101 on result_large_err at src/utils/container_dev/registry.rs:424, a file this PR does not touch and which last changed in #184 - pre-existing, not yours.

One thing I did not put inline because it is a judgement rather than a defect: CHANGELOG.md:96-99, already shipped, says of this same document that "the command never transmits it... the operator's call, on a document they can read first." This PR transmits it by default. That may well be the intended change, but the shipped promise now contradicts the behaviour and one of them should move.

Ten smaller notes held back - a doubled HTTP status in one error render, retry firing on any 4xx, and some test gaps around the new grouping. Say the word and I'll add them.

jetm · 2026-09-04T20:43:52Z

+            .into_iter()
+            .filter(|s| in_runtime(&s.name, &self.runtime))
+            .collect();
+        Ok(cmd.build_document(&kept, &target, snapshot.as_ref()))


uploaded runtime SBOM is named "device SBOM"

scan_runtime_sbom filters scopes with in_runtime and then calls the same build_document, which unconditionally emits "name": "avocado {target} device SBOM" with the comment "Describes what this project installs, and so what runs on the device" (generate.rs:1081-1090), and makes it the sole SpdxDocument.rootElement (generate.rs:1235).

Failure: a project with runtimes dev and prod. avocado connect upload dev attaches a document whose SPDX entry point asserts device-wide coverage while holding only rootfs, initramfs, runtime:dev and ext:dev/*. A consumer traversing rootElement - or taking the first software_Sbom, which is the pattern this PR's own the_device_sbom_is_the_first_one_in_the_graph test pins - CVE-matches a one-runtime inventory as the device inventory, under-reporting every package unique to prod.

The artifact is what a customer consumes, so a name that overstates its scope is worse than a smaller one that is honest about it.

Fixed in 7e4d268. build_document takes runtime: Option<&str>; when set, the root element is avocado <target> runtime <r> SBOM and its comment says other runtimes are not covered. The group loop skips the Group::Runtime that would otherwise repeat the same element under the same name. avocado sbom still passes None and is unchanged. New test: a_runtime_scoped_document_does_not_call_itself_the_device_sbom.

jetm · 2026-09-04T20:43:52Z

+
+    for scope in scopes {
+        // `includes:<n>` is a remote extension, not a project include.
+        if (scope.name.starts_with("ext:") || scope.name.starts_with("includes:"))


nested-layout remote extensions are dropped from every group

sysroot_scan.rs:85 emits a scope named plainly includes - the shared installroot an avocado-ext-layout(nested) extension installs into (ext_fetch.rs:190,207). But groups() keys on includes: and ext:, and in_runtime matches rootfs, initramfs, runtime: and ext:<rt>/. A bare includes matches neither.

Failure: a project whose remote extensions are nested-layout. Their rpmdb lives only in the shared root, so query_root "includes:<n>" finds no database and drops out (sysroot_scan.rs:50,98-101). Those packages then get no per-extension SBOM, appear in no runtime SBOM, and are filtered out of the uploaded runtime.sbom entirely - a shipped extension missing from the artifact, with nothing on the console to say so.

Fixed in 7e4d268. Bare includes is now in in_runtime, alongside rootfs and initramfs, so it lands in every runtime group and in the uploaded slice.

Every runtime rather than none, deliberately: which runtimes actually carry a nested extension needs the declared composition, and that shared root has no rpmdb per extension to split it by. Naming it in one runtime too many is recoverable from the document; dropping it from all of them is not. The ponytail: note now covers both directions and still points at with_composed_config as the upgrade. New test: a_shared_includes_root_rides_along_with_every_runtime.

jetm · 2026-09-04T20:43:52Z

+        let (scopes, target, snapshot) = cmd.scan().await?;
+        let kept: Vec<_> = scopes
+            .into_iter()
+            .filter(|s| in_runtime(&s.name, &self.runtime))


nothing checks the runtime's own scope survived the filter

in_runtime returns true for rootfs and initramfs unconditionally, so kept is non-empty even when no runtime:<name> scope was scanned, or its packages were fully subtracted. build_document then succeeds and build_sbom returns Some with no warning.

Failure: the runtime installroot holds no rpmdb, or its transaction was pure seed - exactly the case a_group_whose_own_scope_is_empty_gets_no_document already pins. groups() emits no Group::Runtime, so Connect stores a document containing not one package that runtime installs, and the operator sees a green upload.

Fixed in 7e4d268. runtime_has_packages sits next to in_runtime — same condition groups skips a runtime on — and scan_runtime_sbom bails on it, so build_sbom warns and the upload goes out without an SBOM rather than with a runtime-named document holding only the base. New test: a_runtime_that_installed_nothing_of_its_own_is_not_reported_as_scanned.

jetm · 2026-09-04T20:43:52Z

-        }
-
-        Ok(())
+        Ok((scopes, target, snapshot))


incomplete-scan warnings are invisible on the JSON path this diff adds

(Anchored at scan()'s return; the two sites are :676 and :684 above.)

scan()'s unreadable-row warning at :676 and seeding_warnings at :684 go out as bare eprintln!, while build_sbom (upload.rs:462) deliberately uses print_warning_above because it also emits {"event":"warning"} on the NDJSON stream.

Failure: avocado connect upload --output json, driven by Avocado Desktop, on a project where three rpm rows carry an embedded tab. An SBOM three packages short is uploaded while the NDJSON stream shows create-runtime: running -> success with no warning event at all.

Worth noting the constraint rather than a straight swap: print_warning_above uses println!, so moving these onto it would corrupt avocado sbom > sbom.json.

Fixed in 7e4d268, and you are right that a straight swap breaks avocado sbom > sbom.json. scan now takes a warn: fn(&str) sink: execute passes |m| eprintln!("[WARN] {m}"), connect upload passes print_warning_above, so the same warning raises a warning event on the NDJSON stream there and stays off stdout here. The [WARN] prefix moved out of the message strings into the sink.

hiagofranco · 2026-09-08T12:13:57Z

All four fixed in 7e4d268 (force-pushed), replies inline.

On the CHANGELOG.md:96-99 contradiction: the promise moved. That paragraph now scopes itself to avocado sbom, which still never transmits, and points at the connect upload entry below for the path that does. The upload entry also gained the runtime-scoped name, the empty-runtime refusal and the warning routing.

cargo test --all-targets green with three added tests; the clean failure locally is the same podman volume-cleanup race, untouched by this. clippy still exits 101 on the pre-existing result_large_err in registry.rs.

Yes to the ten held-back notes, please — the doubled status render especially. The any-4xx retry is deliberate and argued in the commit message, but say so if the argument does not hold.

ENG-2405 emitted one `software_Sbom` covering every sysroot at once, so no runtime or extension had an artifact of its own. Add one per runtime, per extension, and — with --include-sdk — one for the build host, and attach the runtime's own document to `connect upload`. More elements in the one graph rather than one file each. A package in both rootfs and a runtime is one element with two `contains` edges; across files it becomes two, and a scanner counting packages or matching CVEs counts the same exposure twice. A standalone extension file also could not name its parent runtime without an SPDX `imports` map nothing here emits. `build_document` was already a pure function of the scope slice handed to it, so upload builds a runtime-scoped document by filtering rather than by any new CLI surface. `in_runtime` is the one predicate both that filter and the grouping use. A group is skipped when its own defining scope is empty, not when every member is: an empty scope gets no element, so such a group would be named for something the graph never names. The attach never fails an upload. A build error warns and sends nothing, the field is skipped when absent, and any 4xx retries once without it — gated on the status via a new `HttpStatus` error rather than on message text. Any 4xx and not an enumerated list, because the likeliest refusal is 413: ~400 packages of SPDX is megabytes into a body that is otherwise kilobytes. A 4xx also means the first attempt created nothing, so the retry leaves no duplicate runtime. Both notices use `print_warning_above`, since `print_warning` is suppressed under --output json — the one path where a silently SBOM-less upload would go unnoticed. `AVOCADO_UPLOAD_NO_SBOM=1` skips the build. An `includes:<n>` scope is a remote extension, not a project include, so it gets a document too. It and a legacy `ext:<n>` name no runtime, so no runtime group claims them. The bare `includes` scope is the other way round: a nested-layout remote extension installs into that shared root and keeps no database of its own, so its packages are under that one name or nowhere, and `in_runtime` counts them into every runtime rather than dropping them from all of them. The uploaded document is named for the runtime it covers, not for the device: its root element is what a consumer reads coverage off, and a one-runtime slice calling itself the device SBOM under-reports every package the other runtimes install. A runtime that installed nothing of its own sends no SBOM at all — the filtered slice still holds rootfs, so such a document would name the runtime and describe only the base. The scan's own incomplete-read warnings now take the sink the caller passes, so under `connect upload --output json` an SBOM short a package raises a `warning` event on the NDJSON stream instead of only on stderr, while `avocado sbom` keeps stderr and leaves stdout to the document. The device SBOM stays first in the graph — existing tests take the first `software_Sbom` they find — and now states in prose, not only in `software_sbomType`, what it describes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

jetm

Cold re-review at 75ffc1d8. Three blocking findings are inline: two on the SBOM-retry path in connect upload and one on the document namespace in sbom generate.

The theme across the first two is that the retry's trigger and its escape hatch do not line up with what the server actually returns. It fires on statuses that have nothing to do with the SBOM, and it does not fire on the one case it was written for.

Four findings were cut before posting because the open threads on this PR already make the point: the bare-includes over-inclusion argued at generate.rs:204, the document naming at upload.rs:486, the JSON-path warning visibility at generate.rs:738, and the runtime-scope check at upload.rs:484. Fourteen further observations did not clear the bar for a comment and are withheld.

Verified in a scratch worktree rather than read off the diff: cargo test --all-features exits 0 (1520 unit tests plus the integration binaries), cargo fmt --check exits 0, and the namespace collision above was reproduced by running both build_document calls over one parse_scopes output and printing the resulting IRIs.

Two notes on what I did not charge to this change. cargo clippy --all-targets --all-features -- -D warnings fails on clippy::result_large_err at src/utils/container_dev/registry.rs:424, which this diff does not touch. And the SPDX 3.0.1 spec was checked directly on the question of unreachable elements: element and rootElement are both 0..* with no reachability rule, so that observation stayed advisory rather than becoming a conformance finding.

jetm · 2026-09-08T19:22:31Z

+        {
+            Ok(runtime) => Ok((runtime, num_artifacts)),
+            // The server rejecting the SBOM must never fail the upload.
+            Err(e) if create_req.runtime.sbom.is_some() && is_client_error(&e) => {


SBOM retry fires on every 4xx, not just refusals

The retry-without-SBOM arm is gated on is_client_error, which covers the whole 400-499 range. So an expired token, a version already taken, or a rate limit each produce a warning blaming the SBOM and a second POST that fails the same way.

A token expires seconds before avocado connect upload. The first POST returns 401, the operator reads "Connect rejected the runtime with an SBOM attached (...); retrying without it", a second POST goes out with the same dead bearer, returns 401, and that second error is the one that surfaces. ext.rs:23-31 and client.rs:2012-2013 already map 401/403/409/422 as ordinary outcomes of this API, so these are not rare shapes. There is no re-auth path in ConnectClient that would intercept the 401 first.

429 is the worse one: the retry is immediate with no backoff, and if it then succeeds the run exits 0 with no SBOM attached and a warning naming the wrong cause.

The comment's premise that "4xx also means the server created nothing" is an assumption about this server rather than an HTTP guarantee, and 409 falsifies it. Narrowing the arm to the statuses that actually mean "this body was refused" would keep the retry honest.

jetm · 2026-09-08T19:22:31Z

        if !status.is_success() {
            let body = res.text().await.unwrap_or_default();
-            anyhow::bail!("Failed to create runtime (HTTP {status}): {body}");
+            return Err(anyhow::Error::new(HttpStatus(status))


413 as a transport error escapes the retry

HttpStatus is attached only on the status branch here, so a proxy that refuses an oversize body and closes the connection surfaces as a bare transport error. is_client_error then returns false and the upload hard-fails on exactly the case the retry was written for.

A runtime with roughly 400 packages produces a multi-MB SPDX body. The Connect API's fronting proxy (nginx client_max_body_size, or a Plug read_body length cap) answers 413 and closes before the body finishes writing. hyper surfaces Io(BrokenPipe) rather than a status, so .send().await.context("Failed to create runtime")? at lines 1213-1218 produces an error carrying no HttpStatus in its chain, the retry arm at upload.rs:386 is skipped, and the upload aborts on a command that succeeded on the same runtime before this change.

Whether a given proxy yields a status or a broken pipe is deployment- and timing-dependent. The gap in the guard is not.

jetm · 2026-09-08T19:22:31Z

        scopes: &[Scope],
        target: &str,
        snapshot: Option<&RepoSnapshot>,
+        runtime: Option<&str>,


Runtime-scoped doc reuses the device doc spdxIds

build_document derives the document namespace from the target and a digest of the scopes only, so the runtime-scoped document connect upload attaches and the device-wide document avocado sbom writes share {ns}/sbom and {ns}/document while asserting different names and comments.

On a default single-runtime project the two scope sets differ only by empty scopes, and namespace_digest skips those, so the namespaces come out identical. I ran both calls over one parse_scopes output of rootfs plus runtime:dev: both produced the same .../qemuarm64/617f5ae27d4e4a49/sbom IRI, one naming it "avocado qemuarm64 device SBOM" and the other "avocado qemuarm64 runtime dev SBOM", with an identical .../document IRI.

The runtime's own SBOM ends up addressable twice, as {ns}/sbom/runtime-dev-cd6c1d28 in the device document and {ns}/sbom in the uploaded one. Ingesting both into one graph collapses them, which is the merge the namespace_digest comment at lines 879-889 says the digest exists to prevent.

hiagofranco force-pushed the hfranco-eng-2219 branch from f79e9f0 to 658627c Compare September 4, 2026 15:37

hiagofranco requested a review from jetm September 4, 2026 15:37

hiagofranco self-assigned this Sep 4, 2026

jetm reviewed Sep 4, 2026

View reviewed changes

hiagofranco force-pushed the hfranco-eng-2219 branch from 658627c to 7e4d268 Compare September 8, 2026 12:13

hiagofranco force-pushed the hfranco-eng-2219 branch from 7e4d268 to 75ffc1d Compare September 8, 2026 12:14

hiagofranco requested a review from jetm September 8, 2026 12:15

jetm reviewed Sep 8, 2026

View reviewed changes

Uh oh!

Conversation

hiagofranco commented Sep 4, 2026

Summary

Test plan

Uh oh!

jetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiagofranco commented Sep 8, 2026

Uh oh!

jetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants