[Detail Bug] Catalog parsing silently corrupts section/item mappings when XML contains duplicate Ids

Detail Bug Report

https://app.detail.dev/org_62aa40f5-2c23-4914-a665-3bb2068af20e/bugs/bug_00a21343-3067-487f-98d3-e62a35b94b53

Introduced in 5f4f818cf20722982660fe8e00c737353563f2fe by @kewde on Jun 2, 2026

Summary

• Context: parse_catalog_xml builds the catalog browse tree (sections + items + parent→child edges) by recursively walking the IR CatalogSection tree and filling four flat dicts keyed by Id.
• Bug: walk() stores into those dicts unconditionally (sections[s.id]=…, section_to_subsection[s.id]=…, section_to_item[s.id]=…, items[item.id]=…). When a CatalogSection carries an Id already seen, or a CatalogItem carries an Id already seen, the earlier entry is silently overwritten — no error is raised and the corrupt tree is returned to downstream consumers.
• Actual vs. expected: Actual: malformed catalog XML with duplicate section/item Ids is accepted and produces last-write-wins state (silent data corruption). Expected: reject malformed input by raising (at minimum for duplicate Ids within a manufacturer, which the KNX schema enforces).
• Impact:
  • In-memory Registry: duplicate CatalogItem Id causes all sections referencing that id to resolve product/program refs to the surviving item’s metadata; the other listing’s metadata is lost.
  • Persisted DB: _ingest_catalog merges CatalogSectionProduct by a global PK (id=item.id), so a duplicate CatalogItem Id collapses to one DB row and one section silently loses its product row.

Code with Bug

def walk(section: ir.CatalogSection, parent_id: str | None) -> None:
    sections[section.id] = CatalogSection(                                # <-- BUG 🔴 unconditional assignment; duplicate CatalogSection Id overwrites prior section + edge lists
        id=section.id,
        name=section.name,
        number=section.number,
        parent_id=parent_id,
    )
    section_to_subsection[section.id] = [s.id for s in section.catalog_section]
    section_to_item[section.id] = [item.id for item in section.catalog_item]
    for item in section.catalog_item:
        items[item.id] = CatalogItem(                                      # <-- BUG 🔴 unconditional assignment; duplicate CatalogItem Id overwrites prior item metadata
            id=item.id,
            name=item.name,
            number=item.number,
            product_ref_id=item.product_ref_id,
            hardware2_program_ref_id=item.hardware2_program_ref_id,
        )
    for sub in section.catalog_section:
        walk(sub, section.id)

Explanation

• walk() builds a set of flat, id-keyed dictionaries. If the input XML violates the assumed uniqueness of CatalogSection.Id or CatalogItem.Id (at least within a single manufacturer), Python dict assignment silently overwrites earlier entries.
• This creates internally inconsistent state:
  • Duplicate item id: section_to_item for multiple sections still lists the shared id, but items[item.id] contains only the last item, so all lookups resolve to the wrong product/program refs.
  • Duplicate section id: the later section overwrites the earlier section object and its edge lists, potentially leaving some items[...] unreachable from any section.
• There is no upstream XSD validation in load_xml()/load() to catch identity/uniqueness violations, so duplicates are not rejected before walk() runs.

Codebase Inconsistency

• The loader’s docstring documents an invariant of “(ids globally unique)” (packages/product/src/xknxmono/product/loader.py), but walk() does not enforce uniqueness and instead silently clobbers on collision.

Failing Test

def test_parse_catalog_xml_rejects_duplicate_catalog_item_id():
    xml = b"""<?xml version="1.0" encoding="utf-8"?>
<KNX xmlns="http://knx.org/xml/project/23">
  <ManufacturerData>
    <Manufacturer RefId="M-0008">
      <Catalog>
        <CatalogSection Id="M-0008_CS-A" Name="A" Number="1">
          <CatalogItem Id="M-0008_CI-DUP" Name="first" Number="1"
            ProductRefId="M-0008_P-1" Hardware2ProgramRefId="M-0008_HP-1" />
        </CatalogSection>
        <CatalogSection Id="M-0008_CS-B" Name="B" Number="2">
          <CatalogItem Id="M-0008_CI-DUP" Name="second" Number="2"
            ProductRefId="M-0008_P-2" Hardware2ProgramRefId="M-0008_HP-2" />
        </CatalogSection>
      </Catalog>
    </Manufacturer>
  </ManufacturerData>
</KNX>"""
    with pytest.raises(ParseError, match="duplicate CatalogItem Id"):
        parse_catalog_xml(xml)


def test_parse_catalog_xml_rejects_duplicate_catalog_section_id():
    xml = b"""<?xml version="1.0" encoding="utf-8"?>
<KNX xmlns="http://knx.org/xml/project/23">
  <ManufacturerData>
    <Manufacturer RefId="M-0008">
      <Catalog>
        <CatalogSection Id="M-0008_CS-X" Name="first" Number="1" />
        <CatalogSection Id="M-0008_CS-X" Name="second" Number="2" />
      </Catalog>
    </Manufacturer>
  </ManufacturerData>
</KNX>"""
    with pytest.raises(ParseError, match="duplicate CatalogSection Id"):
        parse_catalog_xml(xml)

Recommended Fix

Detect collisions before assignment in walk() and raise ParseError (for duplicate CatalogSection Id and duplicate CatalogItem Id).

History

This bug was introduced in commit 5f4f818. The commit rewrote the product package around an IR-backed ref-id Registry and introduced a brand-new catalog.py whose walk() built flat id-keyed stores by assigning unconditionally.

Start with parse_catalog_xml and its walk() function in the new catalog.py, then read the loader invariant documented in packages/product/src/xknxmono/product/loader.py. Run the two named duplicate-ID tests first. Done means duplicate CatalogSection and CatalogItem IDs raise ParseError instead of overwriting entries.