From c04213eb23e82261048b4b14ea20a2694282a3ed Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 26 Sep 2026 07:49:11 +1000 Subject: [PATCH] PLG: FREEBOX and FRITZBOX didn't respect SET_ALWAYS due to non-matching plugin name in config #1812 --- .claude/skills/plugin-development/SKILL.md | 2 +- .claude/skills/plugin-review/SKILL.md | 2 +- .../skills/plugin-development/plugin-skill.md | 2 +- .gemini/skills/plugin-review/SKILL.md | 2 +- .github/skills/plugin-review/SKILL.md | 2 +- .../skills/plugin-run-development/SKILL.md | 2 +- docs/PLUGINS_DEV.md | 2 +- server/plugins/__template/config.json | 2 +- server/plugins/freebox/config.json | 2 +- server/plugins/fritzbox/config.json | 2 +- test/plugins/test_plugin_conventions.py | 32 +++++++++++++++++++ 11 files changed, 42 insertions(+), 10 deletions(-) diff --git a/.claude/skills/plugin-development/SKILL.md b/.claude/skills/plugin-development/SKILL.md index eb7ee159e..834e69d5a 100644 --- a/.claude/skills/plugin-development/SKILL.md +++ b/.claude/skills/plugin-development/SKILL.md @@ -73,7 +73,7 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext ## Before Opening a PR -Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings instead of duplicating them, description length (renders in the Settings UI — keep it short), the multi-instance settings pattern (nested array + popup-form, see `rest_import`, not a hardcoded "primary"/"secondary" pair), and `allow_raw_text` restricted to display-only column types. Most plugin PR review comments trace back to one of these, and `test/plugins/test_plugin_conventions.py` mechanically enforces the RUN-default, description-length, hardcoded-default-drift, RUN_TIMEOUT-reuse-in-loop, array/object dataType-default_value-mismatch, and allow_raw_text-type-restriction items — run it after touching a plugin. +Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings instead of duplicating them, description length (renders in the Settings UI — keep it short), the multi-instance settings pattern (nested array + popup-form, see `rest_import`, not a hardcoded "primary"/"secondary" pair), `allow_raw_text` restricted to display-only column types, and `scanSourcePlugin`'s static value matching `unique_prefix` exactly. Most plugin PR review comments trace back to one of these, and `test/plugins/test_plugin_conventions.py` mechanically enforces the RUN-default, description-length, hardcoded-default-drift, RUN_TIMEOUT-reuse-in-loop, array/object dataType-default_value-mismatch, allow_raw_text-type-restriction, and scanSourcePlugin-value-mismatch items — run it after touching a plugin. If the plugin needs a new system package or Python dependency, mirroring it into the root `Dockerfile`/`requirements.txt` alone is not enough: see the Conventions Checklist's build-target-mirroring bullet for `.devcontainer/Dockerfile` (regenerate via `.devcontainer/scripts/generate-configs.sh`, don't hand-edit it), `Dockerfile.debian`, and `install/ubuntu24`/`install/proxmox`'s own `requirements.txt` files. diff --git a/.claude/skills/plugin-review/SKILL.md b/.claude/skills/plugin-review/SKILL.md index 555135eaa..c1ef061f6 100644 --- a/.claude/skills/plugin-review/SKILL.md +++ b/.claude/skills/plugin-review/SKILL.md @@ -7,7 +7,7 @@ description: Read when reviewing a plugin PR or auditing an existing plugin scri ## Scope -This is a reviewer-facing checklist, complementary to [[plugin-development]] (which is author-facing). For `config.json` conventions already covered there and mechanically checked by `test/plugins/test_plugin_conventions.py` — `RUN` default, `RUN_TIMEOUT` reuse in a loop, `dataType`/`default_value` agreement, description length — defer to that skill's "Before Opening a PR" checklist rather than re-deriving them here. +This is a reviewer-facing checklist, complementary to [[plugin-development]] (which is author-facing). For `config.json` conventions already covered there and mechanically checked by `test/plugins/test_plugin_conventions.py`, defer to that skill's "Before Opening a PR" checklist and run that test rather than re-deriving the list here - it grows as new checks get added, so a copy of it here would go stale. ## The check this skill adds: no raw SQL in a plugin script diff --git a/.gemini/skills/plugin-development/plugin-skill.md b/.gemini/skills/plugin-development/plugin-skill.md index 76a281b11..406d9ea08 100644 --- a/.gemini/skills/plugin-development/plugin-skill.md +++ b/.gemini/skills/plugin-development/plugin-skill.md @@ -85,7 +85,7 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext ## Before Opening a PR -Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) in `docs/PLUGINS_DEV.md` — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings, description length, the multi-instance settings pattern, and `allow_raw_text` restricted to display-only column types. Most plugin PR review comments trace back to one of these. +Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) in `docs/PLUGINS_DEV.md` — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings, description length, the multi-instance settings pattern, `allow_raw_text` restricted to display-only column types, and `scanSourcePlugin`'s static value matching `unique_prefix` exactly. Most plugin PR review comments trace back to one of these. If the plugin needs a new system package or Python dependency, mirroring it into the root `Dockerfile`/`requirements.txt` alone is not enough: see the Conventions Checklist's build-target-mirroring bullet for `.devcontainer/Dockerfile` (regenerate via `.devcontainer/scripts/generate-configs.sh`, don't hand-edit it), `Dockerfile.debian`, and `install/ubuntu24`/`install/proxmox`'s own `requirements.txt` files. diff --git a/.gemini/skills/plugin-review/SKILL.md b/.gemini/skills/plugin-review/SKILL.md index 555135eaa..c1ef061f6 100644 --- a/.gemini/skills/plugin-review/SKILL.md +++ b/.gemini/skills/plugin-review/SKILL.md @@ -7,7 +7,7 @@ description: Read when reviewing a plugin PR or auditing an existing plugin scri ## Scope -This is a reviewer-facing checklist, complementary to [[plugin-development]] (which is author-facing). For `config.json` conventions already covered there and mechanically checked by `test/plugins/test_plugin_conventions.py` — `RUN` default, `RUN_TIMEOUT` reuse in a loop, `dataType`/`default_value` agreement, description length — defer to that skill's "Before Opening a PR" checklist rather than re-deriving them here. +This is a reviewer-facing checklist, complementary to [[plugin-development]] (which is author-facing). For `config.json` conventions already covered there and mechanically checked by `test/plugins/test_plugin_conventions.py`, defer to that skill's "Before Opening a PR" checklist and run that test rather than re-deriving the list here - it grows as new checks get added, so a copy of it here would go stale. ## The check this skill adds: no raw SQL in a plugin script diff --git a/.github/skills/plugin-review/SKILL.md b/.github/skills/plugin-review/SKILL.md index 5e1b06bd1..7c2bdcb51 100644 --- a/.github/skills/plugin-review/SKILL.md +++ b/.github/skills/plugin-review/SKILL.md @@ -7,7 +7,7 @@ description: Read when reviewing a plugin PR or auditing an existing plugin scri ## Scope -This is a reviewer-facing checklist, complementary to [[plugin-development]] (which is author-facing). For `config.json` conventions already covered there and mechanically checked by `test/plugins/test_plugin_conventions.py` — `RUN` default, `RUN_TIMEOUT` reuse in a loop, `dataType`/`default_value` agreement, description length — defer to that skill's "Before Opening a PR" checklist rather than re-deriving them here. +This is a reviewer-facing checklist, complementary to [[plugin-development]] (which is author-facing). For `config.json` conventions already covered there and mechanically checked by `test/plugins/test_plugin_conventions.py`, defer to that skill's "Before Opening a PR" checklist and run that test rather than re-deriving the list here - it grows as new checks get added, so a copy of it here would go stale. ## The check this skill adds: no raw SQL in a plugin script diff --git a/.github/skills/plugin-run-development/SKILL.md b/.github/skills/plugin-run-development/SKILL.md index a968e6940..4ca3b070a 100644 --- a/.github/skills/plugin-run-development/SKILL.md +++ b/.github/skills/plugin-run-development/SKILL.md @@ -86,7 +86,7 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext ## Before Opening a PR -Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) in `docs/PLUGINS_DEV.md` — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings, description length, the multi-instance settings pattern, and `allow_raw_text` restricted to display-only column types. Most plugin PR review comments trace back to one of these. +Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) in `docs/PLUGINS_DEV.md` — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings, description length, the multi-instance settings pattern, `allow_raw_text` restricted to display-only column types, and `scanSourcePlugin`'s static value matching `unique_prefix` exactly. Most plugin PR review comments trace back to one of these. If the plugin needs a new system package or Python dependency, mirroring it into the root `Dockerfile`/`requirements.txt` alone is not enough: see the Conventions Checklist's build-target-mirroring bullet for `.devcontainer/Dockerfile` (regenerate via `.devcontainer/scripts/generate-configs.sh`, don't hand-edit it), `Dockerfile.debian`, and `install/ubuntu24`/`install/proxmox`'s own `requirements.txt` files. diff --git a/docs/PLUGINS_DEV.md b/docs/PLUGINS_DEV.md index 0c25e5f7e..1ca1d8d00 100755 --- a/docs/PLUGINS_DEV.md +++ b/docs/PLUGINS_DEV.md @@ -243,7 +243,7 @@ Check your plugin against these repo-wide conventions before opening a PR (verif - **Keep `description` strings short.** They render directly in the Settings UI. Put implementation rationale and design trade-offs in the plugin's README or code comments, not the UI-facing description. - **For "one or more instances of the same thing," use the nested array + popup-form settings pattern**, not a fixed hardcoded count (e.g. "primary"/"secondary"). See `rest_import` (`RSTIMPRT`)'s `imports` setting for a working example — it also gives each instance its own sub-settings (URL, credentials, per-instance flags) for free. - **Persist plugin state under `dbFolderPath`, config artifacts under `configPath`** — see [Persisting Plugin Data](#persisting-plugin-data-state--config-files) below. -- **A plugin mapped to `mapped_to_table: "CurrentScan"` must also map `scanSourcePlugin`** (a static value via `mapped_to_column_data`, see [Static Value Mapping](#static-value-mapping) below) — not mechanically enforced by `test_plugin_conventions.py`, so review it by eye. Omitting it leaves `scanSourcePlugin` `NULL` on every row this plugin inserts, which silently breaks two things in `server/scan/device_handling.py`: `create_new_devices()`'s `plugin_prefix = str(scanSourcePlugin).strip() if scanSourcePlugin else "NEWDEV"` mislabels devices this plugin creates as source `NEWDEV`; and `update_devices_data_from_scan()`'s `SELECT DISTINCT scanSourcePlugin FROM CurrentScan` + `[row[0] for row in plugin_rows if row[0]] or [None]` drops the `NULL` rows entirely (the `or [None]` fallback never triggers once any other plugin contributes a non-null prefix), so this plugin's `CurrentScan` rows never get picked up by the per-plugin device-update loop at all — the plugin can *insert* into `CurrentScan` but never actually confirm/update a device's presence. +- **A plugin mapped to `mapped_to_table: "CurrentScan"` must also map `scanSourcePlugin`** (a static value via `mapped_to_column_data`, see [Static Value Mapping](#static-value-mapping) below), **and that value must equal the plugin's own `unique_prefix` exactly.** Whether the mapping is present at all isn't mechanically enforced by `test_plugin_conventions.py`, so review that by eye - but `test_scan_source_plugin_matches_unique_prefix` does mechanically check the value matches `unique_prefix` whenever the mapping exists, since `update_devices_data_from_scan()` (`server/scan/device_handling.py`) reads it verbatim to build the `_SET_ALWAYS`/`_SET_EMPTY` settings keys (`server/db/authoritative_handler.py`) - any mismatch (wrong case, a friendly display name, punctuation) means those settings silently never resolve, so the plugin's SET_ALWAYS/SET_EMPTY overrides get quietly ignored (this shipped for real: `freebox` used `"Freebox"` against `unique_prefix: "FREEBOX"`, `fritzbox` used `"Fritz!Box"` against `"FRITZBOX"`). Omitting the mapping entirely leaves `scanSourcePlugin` `NULL` on every row this plugin inserts, which silently breaks two other things: `create_new_devices()`'s `plugin_prefix = str(scanSourcePlugin).strip() if scanSourcePlugin else "NEWDEV"` mislabels devices this plugin creates as source `NEWDEV`; and `update_devices_data_from_scan()`'s `SELECT DISTINCT scanSourcePlugin FROM CurrentScan` + `[row[0] for row in plugin_rows if row[0]] or [None]` drops the `NULL` rows entirely (the `or [None]` fallback never triggers once any other plugin contributes a non-null prefix), so this plugin's `CurrentScan` rows never get picked up by the per-plugin device-update loop at all - the plugin can *insert* into `CurrentScan` but never actually confirm/update a device's presence. - **A setting's `dataType` and `default_value` must actually agree.** `dataType: "array"` (or `"object"`) means `default_value` must be a real JSON literal for that shape — `'["default"]'`, not the bare string `"default"`. `setting_value_to_python_type()` (`server/helper.py`) `json.loads()`s the default at runtime; a bare string fails that parse, silently logs a decode error, and returns `[]` instead of your intended default — this shipped for real in `devParentRelType`/`UI_theme`/`UI_TOPOLOGY_ORDER` before being caught. If `elementOptions` already sets `multiple`/`orderable: "false"`, that's a strong signal the setting is actually scalar and `dataType` should be `"string"`, not `"array"`, regardless of what UI widget (`select`, etc.) renders it. - **Only set `"allow_raw_text": true` on a display-only column type (`textarea_readonly`).** Every plugin-sourced field is HTML/control-char-stripped by default before it reaches the DB; see [Field Sanitization](#field-sanitization) below. `test_allow_raw_text_only_on_safe_types` (`test/plugins/test_plugin_conventions.py`) enforces the type restriction; it can't catch a column that legitimately needs the opt-out but is rendered somewhere unsafe, so use it only for values that are never interpreted as HTML. - **A new system package or Python dependency needs mirroring across every build target it should work on, not just the root `Dockerfile`.** `.devcontainer/Dockerfile` is auto-generated (its own header says so) by `.devcontainer/scripts/generate-configs.sh`, which concatenates the root `Dockerfile` with `.devcontainer/resources/devcontainer-Dockerfile`. It's a committed, git-tracked file, not something read fresh at build time - a package added to the root `Dockerfile` without re-running that script leaves the devcontainer's checked-in Dockerfile stale, so the package is silently missing there until someone regenerates it. `Dockerfile.debian` (`docs/BUILDS.md`) is a second, separately-maintained build target with its own `apt-get install` package list and `setcap` calls - a system package (and its `setcap` grant, if it needs one) has to be added there too, not just to the Alpine `Dockerfile`. For a new Python dependency, the root `requirements.txt` is not the only one either: `install/ubuntu24/requirements.txt` and `install/proxmox/requirements.txt` are separate lists for their respective non-Docker install methods, not mechanically kept in sync with the root file (no CI check covers this) - check whether the new dependency is actually needed by those install methods too rather than assuming the root file alone is enough. diff --git a/server/plugins/__template/config.json b/server/plugins/__template/config.json index 413bf869f..bbc450651 100755 --- a/server/plugins/__template/config.json +++ b/server/plugins/__template/config.json @@ -652,7 +652,7 @@ "column": "Dummy", "mapped_to_column": "scanSourcePlugin", "mapped_to_column_data": { - "value": "Example Plugin" + "value": "TMP" }, "css_classes": "col-sm-2", "show": false, diff --git a/server/plugins/freebox/config.json b/server/plugins/freebox/config.json index f605defa7..91e041760 100755 --- a/server/plugins/freebox/config.json +++ b/server/plugins/freebox/config.json @@ -504,7 +504,7 @@ "column": "Dummy", "mapped_to_column": "scanSourcePlugin", "mapped_to_column_data": { - "value": "Freebox" + "value": "FREEBOX" }, "css_classes": "col-sm-2", "show": false, diff --git a/server/plugins/fritzbox/config.json b/server/plugins/fritzbox/config.json index df52d26ad..9f210da68 100755 --- a/server/plugins/fritzbox/config.json +++ b/server/plugins/fritzbox/config.json @@ -2726,7 +2726,7 @@ "column": "Dummy", "mapped_to_column": "scanSourcePlugin", "mapped_to_column_data": { - "value": "Fritz!Box" + "value": "FRITZBOX" }, "css_classes": "col-sm-2", "show": false, diff --git a/test/plugins/test_plugin_conventions.py b/test/plugins/test_plugin_conventions.py index 69188f177..2a4fce918 100644 --- a/test/plugins/test_plugin_conventions.py +++ b/test/plugins/test_plugin_conventions.py @@ -393,3 +393,35 @@ def test_allow_raw_text_only_on_safe_types(plugin_name): 'still requires the renderer to escape on display - see ' 'docs/PLUGINS_DEV.md#conventions-checklist.' ) + + +@pytest.mark.parametrize('plugin_name', _PLUGIN_NAMES) +def test_scan_source_plugin_matches_unique_prefix(plugin_name): + """A CurrentScan-mapped plugin's static scanSourcePlugin value must equal + its own unique_prefix exactly. update_devices_data_from_scan() + (server/scan/device_handling.py) reads this value straight out of + CurrentScan and uses it verbatim to build the '_SET_ALWAYS' / + '_SET_EMPTY' settings keys (server/db/authoritative_handler.py) - + any mismatch (wrong case, a friendly display name, punctuation) means + those settings silently never match, so the plugin's SET_ALWAYS/SET_EMPTY + overrides are quietly ignored. There's no legitimate reason for this + value to differ from unique_prefix; it's never shown to the user.""" + config = _load_config(plugin_name) + if config.get('mapped_to_table') != 'CurrentScan': + return + + prefix = config.get('unique_prefix') + for col in config.get('database_column_definitions', []): + if col.get('mapped_to_column') != 'scanSourcePlugin': + continue + value = (col.get('mapped_to_column_data') or {}).get('value') + if value is None: + continue + assert value == prefix, ( + f"{plugin_name}: scanSourcePlugin's static value is {value!r}, but " + f"unique_prefix is {prefix!r}. These must match exactly - " + f"update_devices_data_from_scan() uses scanSourcePlugin's value verbatim " + f"to look up '{value}_SET_ALWAYS'/'{value}_SET_EMPTY', which silently never " + f"resolves to the real '{prefix}_SET_ALWAYS'/'{prefix}_SET_EMPTY' settings " + "if the two differ." + )