From 71912f0515e974562d1610687d39b04c6cd71a98 Mon Sep 17 00:00:00 2001 From: Ville Laitila Date: Thu, 27 Aug 2026 22:13:45 +0300 Subject: [PATCH 1/3] SBOM: make group the parent's name, as CycloneDX defines the field group carried the parent's full path so that two identically named groups under different roots stayed apart. That guarded a collision no measured estate has, at two costs every estate pays. CycloneDX defines group as "a shortened, single name of the company or project that produced the component" and asks that special characters be avoided. `/Estate/AccessManagement` is neither, and a consumer rendering group as a package coordinate shows a path. The heavier cost is stability. The first path segment is the estate root, the least stable part of the path, and group is an identity field for consumers such as Dependency-Track. Renaming an estate therefore re-identified every component beneath it at once -- observed in a live estate across 689 documents when its root was renamed. Against that, the disambiguation was not observed at all: across ten real estates every full-path group mapped one-to-one onto its bare name. Nor did it hold up in our own tests. The case documented as "the case the feature exists for", test_inlined_mirror_is_told_apart_from_its_host_by_its_ published_location, passes unchanged with bare names, because what separates its two 'shared' components is the parent names GroupA and GroupB, not the paths. The collision is now accepted knowingly and covered by a test that states it: two same-named groups under different roots share a group value, and softagram:elementPath and the document serial still tell them apart. The full path is not lost -- elementPath publishes it verbatim, and consumers who parsed it out of group should read it there. docs/data-formats.md carried both problems as caveats rather than fixes; those are replaced by a description of the trade actually made. The 2026-08-11 design spec is marked partly superseded rather than rewritten. --- docs/data-formats.md | 53 ++++--- .../2026-08-11-sbom-element-path-design.md | 7 + .../converters/sbom_cyclonedx_generator.py | 21 ++- .../sbom_cyclonedx_generator_test.py | 139 ++++++++++++++++-- 4 files changed, 173 insertions(+), 47 deletions(-) diff --git a/docs/data-formats.md b/docs/data-formats.md index 32e7198..9204459 100644 --- a/docs/data-formats.md +++ b/docs/data-formats.md @@ -537,7 +537,7 @@ and every internal component — publishes its position in the model: | Field | Meaning | |-------|---------| -| `group` | The **full path of the parent element**, not just its name. Omitted for a top-level element, which has no parent path. | +| `group` | The **name of the parent element**. Omitted for a top-level element, whose parent is the model root and has no name. | | `properties[softagram:elementPath]` | The element's own full path. A property rather than a field because the CycloneDX component schema sets `additionalProperties: false`. | | `properties[softagram:elementType]` | What the **model** calls this element — `repository`, `dir`, `file`, and so on. **Present only when the model carries a type**; see below. | | `externalReferences[type=vcs]` | The `repo_url` of the element, or of the **nearest ancestor** carrying a non-blank one. Absent when no ancestor has one — never a placeholder. | @@ -581,37 +581,36 @@ Components describing **3rd-party packages** carry none of these. Their identity single-SBOM document. This is long-standing behaviour rather than a recent change, and altering it would change the identity every existing single-SBOM consumer files the document under, so it is documented here rather than quietly repaired. -- `group + '/' + name == elementPath` below the top level. -- Two repositories that share a name under different groups are distinguished by `group` and - `elementPath`. They are *not* reliably distinguished by `bom-ref`, whose collision suffix - (`repoa`, `repoa-2`) depends on traversal order and can change between model generations. +- `elementPath` ends with `'/' + group + '/' + name` below the top level. `group` is a single + segment of the path, not the whole of it — read `elementPath` when you need the location. +- Two repositories that share a name under **differently named** groups are distinguished by + `group` and `elementPath`; under **identically named** groups in different parts of the tree, + by `elementPath` alone. They are *not* reliably distinguished by `bom-ref`, whose collision + suffix (`repoa`, `repoa-2`) depends on traversal order and can change between model + generations. ### Caveats - **The first path segment is the estate root and is not stable.** It changes when the estate is renamed or restructured. Read it from the path rather than hardcoding it. -- **`group` holds a path, not a package namespace.** The CycloneDX specification suggests - avoiding special characters in `group` and shows package coordinates such as - `org.apache.commons`. A model group is a tree location, so this converter puts the parent's - full path there. Tools that render `group` as a package coordinate will show the path. - - **What the path buys, stated accurately.** It distinguishes two *groups* that share a name under - different parents — `/Estate/TeamA/tools` and `/Estate/TeamB/tools` — *in a model that has such - a pair*. Many models have none. Measured across three real single-root estates at both level 2 - and level 3, every full-path `group` value mapped one-to-one onto its bare name (73 → 73, 78 → - 78, 11 → 11), so the prefix disambiguated nothing in any of them. - - So the path is a guarantee that holds for *every* model, not a fix for a collision every model - has, and a reader should not infer that their own `group` values would collide without it. Note - the collision it guards against is between **groups**; two repositories sharing a name under - *differently* named groups are already distinguished by the group name alone. - - **If you want the bare name, take it from the path**; the last segment is the parent's name. - And if you want an unambiguous identifier for the element itself, use - `properties[softagram:elementPath]`, which is unique across all documents from one model and is - the string the serial number is derived from — `group` is not the field to reach for. Note the - first path segment is the estate root and changes when the estate is renamed, so anything - derived from `group` inherits that instability. +- **`group` does not identify an element on its own.** It is the parent's name, so two groups + that share a name in different parts of the tree — `/Estate/TeamA/tools` and + `/Estate/TeamB/tools` — produce the same `group`. Use `properties[softagram:elementPath]` when + you need an unambiguous identifier: it is unique across all documents from one model and is the + string the serial number is derived from. + + **This is a deliberate trade, changed in 1.17.0.** `group` previously carried the parent's full + path, which kept such a pair apart. Two things made that the worse deal. CycloneDX defines + `group` as "a shortened, single name of the company or project that produced the component" and + asks that special characters be avoided, so a path was not what a consumer reading the field + expects. More seriously, the first path segment is the estate root — the least stable part of + the path — and `group` is an identity field, so renaming an estate re-identified every component + beneath it at once. The collision the path guarded against was not observed in any of ten real + estates measured, where every full-path `group` mapped one-to-one onto its bare name; the rename + cost was observed, across 689 documents in a live estate. + + **Consumers who parsed the path out of `group` should read `softagram:elementPath` instead**, + which is unchanged and still carries the full path. - **`purl` and `version` are empty on the metadata component**, and on an internal component whose element publishes no unambiguous package. A repository has no package identity and no version of its own; a path is not a valid purl and is deliberately not placed there. An diff --git a/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md b/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md index 59c5e90..9f7d87e 100644 --- a/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md +++ b/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md @@ -4,6 +4,13 @@ Design for a change to `src/sgraph/converters/sbom_cyclonedx_generator.py`. Baseline: sgraph **1.9.0** (`upstream/main` @ `fb65e0d`), 237 tests green. +> **Partly superseded in 1.17.0.** This document specifies `group` as the parent element's +> *full path*. That was changed to the parent's *bare name*: the path put the unstable estate +> root inside an identity field, and CycloneDX defines `group` as a single name rather than a +> location. The rest of the design — `softagram:elementPath`, the derived serial, the vcs +> ancestor walk — is unchanged. See "Caveats" in `docs/data-formats.md`. This file is kept as +> the record of the original decision. + --- ## 1. The problem diff --git a/src/sgraph/converters/sbom_cyclonedx_generator.py b/src/sgraph/converters/sbom_cyclonedx_generator.py index e05a2e6..31ce62a 100644 --- a/src/sgraph/converters/sbom_cyclonedx_generator.py +++ b/src/sgraph/converters/sbom_cyclonedx_generator.py @@ -930,9 +930,22 @@ def analyze_3rdparty(external_root, sbom): def _add_element_location(component, elem): """Publish where elem sits in the model, and what the model calls it, on a component for elem. - 'group' carries the parent's full path rather than its bare name so that two identically - named groups under different roots stay distinguishable, and is omitted for a top-level - element, whose parent is the model root and has no path of its own. + 'group' carries the parent's bare name, and is omitted for a top-level element, whose + parent is the model root and has no name of its own. + + It used to carry the parent's full path, to keep two identically named groups under + different roots apart. That traded a collision no measured estate had for two costs every + estate paid. CycloneDX defines group as "a shortened, single name of the company or + project that produced the component" and asks that special characters be avoided, so a + value like '/Estate/AccessManagement' is not what a consumer reading the field expects. + Worse, the first path segment is the estate root -- the least stable part of the path -- + and group is an identity field, so renaming an estate re-identified every component + beneath it at once. That happened to a live estate across 689 documents. + + The collision the path guarded against is now accepted knowingly: two same-named groups + under different roots share a group value. Identity does not rest on it. The full path is + still published verbatim as softagram:elementPath, the document serial is derived from + that path, and both still tell such a pair apart. The element's own path goes into a property because CycloneDX sets additionalProperties: false on component, leaving properties[] as the only schema-valid place for it. It is also @@ -961,7 +974,7 @@ def _add_element_location(component, elem): """ parent_path = elem.parent.getPath() if parent_path: - component['group'] = parent_path + component['group'] = parent_path.rsplit('/', 1)[-1] component.setdefault('properties', []).append({ 'name': ELEMENT_PATH_PROPERTY, 'value': elem.getPath() diff --git a/tests/converters/sbom_cyclonedx_generator_test.py b/tests/converters/sbom_cyclonedx_generator_test.py index 8f1e5e8..96d0634 100644 --- a/tests/converters/sbom_cyclonedx_generator_test.py +++ b/tests/converters/sbom_cyclonedx_generator_test.py @@ -1639,13 +1639,13 @@ def test_metadata_component_carries_element_path(): assert find_property(component, 'softagram:elementPath') == '/OrgName/GroupA/repoA' -def test_metadata_component_carries_the_parent_path_as_group(): - """group holds the parent's full path, so two same-named groups stay distinguishable.""" +def test_metadata_component_carries_the_parent_name_as_group(): + """group holds the parent's bare name, which is what CycloneDX asks the field to hold.""" model, _ = get_model_and_model_api(MULTI_MODEL) result = generate_multi_from_sgraph(model, level=3) - assert sbom_of(result, 'repoA')['metadata']['component']['group'] == '/OrgName/GroupA' - assert sbom_of(result, 'repoB')['metadata']['component']['group'] == '/OrgName/GroupA' + assert sbom_of(result, 'repoA')['metadata']['component']['group'] == 'GroupA' + assert sbom_of(result, 'repoB')['metadata']['component']['group'] == 'GroupA' def test_element_path_matches_the_serial_number_for_every_sbom(): @@ -1669,7 +1669,7 @@ def test_element_location_is_level_agnostic(): result = generate_multi_from_sgraph(model, level=2) component = sbom_of(result, 'GroupA')['metadata']['component'] - assert component['group'] == '/OrgName' + assert component['group'] == 'OrgName' assert find_property(component, 'softagram:elementPath') == '/OrgName/GroupA' @@ -1692,7 +1692,7 @@ def test_selected_element_sbom_also_carries_its_location(): sbom = generate_for_element_from_sgraph(model, '/OrgName/GroupA/repoA/src') component = sbom['metadata']['component'] - assert component['group'] == '/OrgName/GroupA/repoA' + assert component['group'] == 'repoA' assert find_property(component, 'softagram:elementPath') == '/OrgName/GroupA/repoA/src' @@ -1713,11 +1713,11 @@ def test_mirrored_repositories_are_distinguished_by_their_location(): assert [c['name'] for c in components] == ['shared', 'shared'] by_group = {c['group']: c for c in components} - assert sorted(by_group) == ['/OrgName/GroupA', '/OrgName/GroupB'] + assert sorted(by_group) == ['GroupA', 'GroupB'] - assert find_property(by_group['/OrgName/GroupA'], 'softagram:elementPath') \ + assert find_property(by_group['GroupA'], 'softagram:elementPath') \ == '/OrgName/GroupA/shared' - assert find_property(by_group['/OrgName/GroupB'], 'softagram:elementPath') \ + assert find_property(by_group['GroupB'], 'softagram:elementPath') \ == '/OrgName/GroupB/shared' # The collision suffix keeps bom-refs unique within the set, but does not identify either one @@ -1764,9 +1764,9 @@ def test_mirrored_repositories_carry_their_own_distinct_repository_urls(): by_group = {sbom['metadata']['component']['group']: sbom['metadata']['component'] for sbom in result} - assert [r['url'] for r in by_group['/OrgName/GroupA']['externalReferences'] + assert [r['url'] for r in by_group['GroupA']['externalReferences'] if r['type'] == 'vcs'] == ['https://example.org/org/groupa-shared.git'] - assert [r['url'] for r in by_group['/OrgName/GroupB']['externalReferences'] + assert [r['url'] for r in by_group['GroupB']['externalReferences'] if r['type'] == 'vcs'] == ['https://example.org/org/groupb-shared.git'] @@ -1780,7 +1780,7 @@ def test_nearest_repo_url_wins_over_a_more_distant_ancestor(): result = generate_multi_from_sgraph(model, level=3) component = next(s['metadata']['component'] for s in result - if s['metadata']['component']['group'] == '/OrgName/GroupA') + if s['metadata']['component']['group'] == 'GroupA') vcs = [r['url'] for r in component['externalReferences'] if r['type'] == 'vcs'] assert vcs == ['https://example.org/org/groupa-shared.git'] @@ -1807,7 +1807,7 @@ def test_transitive_internal_components_carry_their_location(): result = generate_multi_from_sgraph(model, level=3, transitive=True) repo_b = next(c for c in sbom_of(result, 'repoA')['components'] if c['name'] == 'repoB') - assert repo_b['group'] == '/OrgName/GroupA' + assert repo_b['group'] == 'GroupA' assert find_property(repo_b, 'softagram:elementPath') == '/OrgName/GroupA/repoB' # The pre-existing internal marker survives alongside the new property assert find_property(repo_b, 'softagram:internal') == 'true' @@ -1825,12 +1825,12 @@ def test_inlined_mirror_is_told_apart_from_its_host_by_its_published_location(): model, _ = get_model_and_model_api(MIRRORED_MODEL) result = generate_multi_from_sgraph(model, level=3, transitive=True) - host = next(s for s in result if s['metadata']['component']['group'] == '/OrgName/GroupA') + host = next(s for s in result if s['metadata']['component']['group'] == 'GroupA') inlined = next(c for c in host['components'] if find_property(c, 'softagram:internal') == 'true') assert inlined['name'] == host['metadata']['component']['name'] == 'shared' - assert inlined['group'] == '/OrgName/GroupB' + assert inlined['group'] == 'GroupB' assert find_property(inlined, 'softagram:elementPath') == '/OrgName/GroupB/shared' # List comprehensions rather than a {type: url} dict: these pin cardinality too, so a @@ -1838,7 +1838,7 @@ def test_inlined_mirror_is_told_apart_from_its_host_by_its_published_location(): vcs = [r['url'] for r in inlined['externalReferences'] if r['type'] == 'vcs'] assert vcs == ['https://example.org/org/groupb-shared.git'] - mirror = next(s for s in result if s['metadata']['component']['group'] == '/OrgName/GroupB') + mirror = next(s for s in result if s['metadata']['component']['group'] == 'GroupB') mirror_serial = mirror['serialNumber'].replace('urn:uuid:', '') bom_links = [r['url'] for r in inlined['externalReferences'] if r['type'] == 'bom'] assert bom_links == [f'urn:cdx:{mirror_serial}/1'] @@ -4402,3 +4402,110 @@ def test_cli_rejects_an_unsupported_spec_version(tmp_path): assert '--spec-version' in proc.stderr for supported in SUPPORTED_SPEC_VERSIONS: assert supported in proc.stderr, proc.stderr + + +# --- group is a name, not a path --- +# +# 'group' used to carry the parent's full path. Three things were wrong with that, and the +# first two were already conceded in docs/data-formats.md as caveats rather than repaired: +# +# 1. CycloneDX defines group as "a shortened, single name of the company or project that +# produced the component" and says "whitespace and special characters should be +# avoided". A value like '/Estate/AccessManagement' is neither. +# 2. The first path segment is the estate root, the least stable part of the path. Because +# group is an identity field, renaming an estate rewrote the identity of every component +# in every document at once. That is not hypothetical: a reporting estate renamed its +# root and saw exactly this across 689 documents. +# 3. The disambiguation it was said to buy was not measured in any real estate. Across ten +# of them every full-path group mapped one-to-one onto its bare name. +# +# The full path remains published, in softagram:elementPath, which is the schema-lawful place +# for it and is what a consumer needing a tree location should read. + + +def test_group_is_a_name_not_a_path(): + """A path in group contradicts what CycloneDX says the field is for.""" + model, _ = get_model_and_model_api(MIRRORED_MODEL) + + documents = generate_multi_from_sgraph(model, level=3, transitive=True) + + groups = [d['metadata']['component']['group'] for d in documents + if d['metadata']['component'].get('group')] + assert groups, 'fixture produced no group values to check' + for group in groups: + assert '/' not in group, ( + f'group {group!r} is a path; CycloneDX asks for a single name with special ' + f'characters avoided, and the full path is published as softagram:elementPath') + + +def test_component_group_is_a_name_not_a_path(): + """The same holds for components inside a document, not only its metadata.""" + model, _ = get_model_and_model_api(MIRRORED_MODEL) + + documents = generate_multi_from_sgraph(model, level=3, transitive=True) + + for document in documents: + for component in document.get('components', []): + if component.get('group'): + assert '/' not in component['group'], component['group'] + + +def _estate(root_name): + """Two groups under one estate root, each holding a repository.""" + model = SGraph(SElement(None, '')) + for group_name in ('TeamA', 'TeamB'): + element = model.createOrGetElementFromPath(f'/{root_name}/{group_name}/service') + element.attrs['type'] = 'repository' + return model + + +def test_group_is_unchanged_by_an_estate_root_rename(): + """Renaming the estate must not rewrite the identity of every component in it. + + group is an identity field for consumers such as Dependency-Track. Carrying the estate + root inside it meant an organisational rename -- the least stable segment of the path -- + silently re-identified everything below it. + """ + before = generate_multi_from_sgraph(_estate('TalenomSoftware'), level=3, transitive=True) + after = generate_multi_from_sgraph(_estate('SaaS'), level=3, transitive=True) + + groups_before = sorted(d['metadata']['component'].get('group', '') for d in before) + groups_after = sorted(d['metadata']['component'].get('group', '') for d in after) + + assert groups_before == groups_after, ( + f'an estate rename changed every group value: {groups_before} -> {groups_after}') + + +def test_the_estate_root_is_still_recoverable_after_the_change(): + """Dropping the path from group must not lose it -- elementPath still carries it.""" + documents = generate_multi_from_sgraph(_estate('SaaS'), level=3, transitive=True) + + paths = [find_property(d['metadata']['component'], 'softagram:elementPath') + for d in documents] + assert all(p and p.startswith('/SaaS/') for p in paths), paths + + +def test_groups_sharing_a_name_under_different_roots_stay_distinguishable(): + """The cost of the change, made explicit and shown to be covered. + + A bare name is all that separates two same-named groups under different roots, so their + group values now collide where the full path kept them apart. That collision is accepted + deliberately: identity does not rest on group. elementPath and the document serial both + still separate them, and neither was affected by this change. + """ + model = SGraph(SElement(None, '')) + for root in ('RootX', 'RootY'): + element = model.createOrGetElementFromPath(f'/{root}/Shared/service') + element.attrs['type'] = 'repository' + + documents = generate_multi_from_sgraph(model, level=3, transitive=True) + components = [d['metadata']['component'] for d in documents] + + groups = {c.get('group') for c in components} + assert groups == {'Shared'}, f'expected the collision this change accepts, got {groups}' + + paths = {find_property(c, 'softagram:elementPath') for c in components} + assert paths == {'/RootX/Shared/service', '/RootY/Shared/service'}, paths + + serials = {d['serialNumber'] for d in documents} + assert len(serials) == len(documents), 'document serials must still be distinct' From aae4dddaa92574feb175418ba6a12473960db2e6 Mon Sep 17 00:00:00 2001 From: Ville Laitila Date: Fri, 28 Aug 2026 00:01:44 +0300 Subject: [PATCH 2/3] SBOM: read the parent's name directly instead of re-deriving it from the path --- src/sgraph/converters/sbom_cyclonedx_generator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sgraph/converters/sbom_cyclonedx_generator.py b/src/sgraph/converters/sbom_cyclonedx_generator.py index 31ce62a..046f2bf 100644 --- a/src/sgraph/converters/sbom_cyclonedx_generator.py +++ b/src/sgraph/converters/sbom_cyclonedx_generator.py @@ -972,9 +972,9 @@ def _add_element_location(component, elem): elementPath yields a document a consumer cannot resolve and cannot detect as broken. Raising at the call site is the better failure. """ - parent_path = elem.parent.getPath() - if parent_path: - component['group'] = parent_path.rsplit('/', 1)[-1] + parent_name = elem.parent.name + if parent_name: + component['group'] = parent_name component.setdefault('properties', []).append({ 'name': ELEMENT_PATH_PROPERTY, 'value': elem.getPath() From 098141ce17873214bc9595e9b5de292c78d9a4e6 Mon Sep 17 00:00:00 2001 From: Ville Laitila Date: Fri, 28 Aug 2026 00:12:21 +0300 Subject: [PATCH 3/3] docs: group examples carry the parent's bare name, matching the field they illustrate --- docs/data-formats.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/data-formats.md b/docs/data-formats.md index 9204459..dfe7247 100644 --- a/docs/data-formats.md +++ b/docs/data-formats.md @@ -385,7 +385,7 @@ document's own subject, and not a 3rd-party package. It appears as a component m "name": "ui-lib", "version": "2.1.0", "purl": "pkg:generic/ui-lib@2.1.0", - "group": "/OrgName/GroupA", + "group": "GroupA", "properties": [ { "name": "softagram:internal", "value": "true" }, { "name": "softagram:packageName", "value": "ui-lib" }, @@ -522,7 +522,7 @@ and every internal component — publishes its position in the model: ```json { "bom-ref": "repoa", "name": "repoA", - "group": "/OrgName/GroupA", + "group": "GroupA", "type": "application", "version": "", "purl": "",