PLG: FREEBOX and FRITZBOX didn't respect SET_ALWAYS due to non-matching plugin name in config #1812

.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.
 

.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
 

.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.
 

.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
 

.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
 

.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.
 

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 `<value>_SET_ALWAYS`/`<value>_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.

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,

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,

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,

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 '<value>_SET_ALWAYS' /
+    '<value>_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."
+        )