XKNX / XKNX/xknxtoolkit

[Detail Bug] KNX GUI: Loading malformed .knxprod crashes the app instead of showing an in-app import error

Open Beginner friendly
#97 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
4
Forks
0
Avg merge
15h 38m
Merged PRs (30d)
37

Description

Detail Bug Report

https://app.detail.dev/org_62aa40f5-2c23-4914-a665-3bb2068af20e/bugs/bug_6c95205f-1906-4317-b370-21154d9febe7

Introduced in e066b2be9f21453920a1851fe9853334e1a3f96b by @kewde on May 9, 2026

Summary

  • Context: KnxGuiApp._load_knxprod (apps/knx-gui/src/knx_gui/main.py) is invoked from the File → "Load knxprod…" menu when a user selects a .knxprod file; it registers a background task on TaskService and imports the file into the catalog.
  • Bug: _load_knxprod already has exception handlers that convert import failures into recoverable in-app error badges + logged error lines — except ArchiveError and except (OSError, ValueError) each call self._log.error(...) and self._task_service.update(task_id, status="error", ...). The code's own intent, expressed by these existing handlers, is that a failed import recovers within the GUI rather than crashing. The bug is that this recovery is incomplete: CatalogService.import_knxprod can also raise xknxmono.models.schema.VersionError, zipfile.BadZipFile, and xknxmono.product.errors.ParseError — all confirmed siblings of ArchiveError/OSError/ValueError, not subclasses — so none are caught, and the exception propagates out of _load_knxprodgui_menucallbacks.show_menushello_imgui.run(), which re-raises and terminates the GUI process (verified end-to-end under xvfb, Evidence §2; reproducer scripts included in Appendix A).
  • Actual vs. expected: Expected — a failed import produces a red ERROR log row in the Logger panel and an error status-bar badge, exactly as the existing handlers already do for ArchiveError/OSError/ValueError. Actual — loading a malformed .knxprod via the File menu crashes the GUI application instead.
  • Impact: The harm is a UX regression: loading a malformed .knxprod crashes the GUI application (process exits) where the codebase's own existing handlers for this exact call (except ArchiveError, except (OSError, ValueError)) already establish the pattern of recovering with an in-app error badge + logged error line.

Code with Bug

def _load_knxprod(self, path: str) -> None:
    self._log.info("loading knxprod", path=path)
    task_id = self._task_service.add(f"Loading {Path(path).name}")
    try:
        added = self._catalog_service.import_knxprod(Path(path))
        if added:
            self._log.info("added applications to catalog", count=len(added))
        else:
            self._log.info("no new applications", path=path)
        self._task_service.remove(task_id)
    except ArchiveError as e:
        self._log.error("archive error", path=path, error=str(e))
        self._task_service.update(task_id, status="error", detail=str(e))
    except (OSError, ValueError) as e:
        self._log.error("import error", path=path, error=f"{type(e).__name__}: {e}")
        self._task_service.update(task_id, status="error", detail=str(e))
        # <-- BUG 🔴 VersionError/BadZipFile/ParseError are not caught, so they escape and crash the GUI

Explanation

  • CatalogService.import_knxprod calls into xknxmono parsing code that can raise xknxmono.models.schema.VersionError (version detection fails on structurally-valid-but-invalid XML), zipfile.BadZipFile (CRC mismatch when reading a ZIP member), and xknxmono.product.errors.ParseError (semantic XML validation failure). These are not ArchiveError, OSError, or ValueError subclasses, so they are not handled.
  • In the resolved imgui-bundle version (1.92.700), an exception raised from the menu callback path is re-raised by hello_imgui.run(), so the process terminates rather than continuing the GUI loop.
  • Because the try block has no finally, an uncaught exception also skips both TaskService.remove() and TaskService.update(), leaving the task in status="running" until the process exits.

Recommended Fix

Add a narrow handler for the three demonstrated escaping exception types (do not broaden to except Exception, since the import path also does DB and filesystem work):

import zipfile
from xknxmono.models.schema import VersionError
from xknxmono.product.errors import ArchiveError, ParseError
...
        except ArchiveError as e:
            self._log.error("archive error", path=path, error=str(e))
            self._task_service.update(task_id, status="error", detail=str(e))
        except (OSError, ValueError) as e:
            self._log.error("import error", path=path, error=f"{type(e).__name__}: {e}")
            self._task_service.update(task_id, status="error", detail=str(e))
        except (VersionError, ParseError, zipfile.BadZipFile) as e:
            self._log.error("import error", path=path, error=f"{type(e).__name__}: {e}")
            self._task_service.update(task_id, status="error", detail=str(e))

History

This bug was introduced in commit e066b2b. That commit ("feat(knx-gui): add menu entry to load .knxprod files with stub popup", Sat May 9 2026) stubbed the initially-stub _load_knxprod — opening a .knxprod file and rendering a popup with its contents — and chose exactly except ArchiveError / except (OSError, ValueError) to cover the failure modes of the then-xknx ProductArchive API; the bug slipped in because that handler set never accounted for VersionError (from version detection on a structurally-valid-but-corrupt archive), the product.errors.ParseError (distinct from the caught xsdata.exceptions.ParserError), or zipfile.BadZipFile (from CRC mismatch on a member that passes the namelist check), so when the import path was later rewritten onto xknxmono.catalog/xknxmono.product the same narrow shape was carried forward unchanged and remains the missing-from-the-handler-set bug today. (git blame points to 480c139 on the handler lines, but that commit is a mechanical chore: ruff knx gui mass-reformat that preserves the logic verbatim; PR #36 / ac577ad later added only the task_id lifecycle wiring on top, mirroring the existing narrow handler shape rather than widening it.)

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in apps/knx-gui/src/knx_gui/main.py at KnxGuiApp._load_knxprod and review its existing exception handlers and TaskService updates. Reproduce the malformed .knxprod cases described in Appendix A, then verify that VersionError, ParseError, and BadZipFile produce the existing error log and status-bar badge without terminating the GUI.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
desktop
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.