From 6ab220fd8c6f4866e571480c3edd2d8ed0cbbb66 Mon Sep 17 00:00:00 2001
From: jokob-sk <jokob.sk@gmail.com>
Date: Mon, 14 Sep 2026 18:17:06 +1000
Subject: [PATCH 1/2] BE+FE: async event execution

---
 CLAUDE.md                                 |  2 +-
 front/js/common.js                        |  8 ++-
 front/js/devices-table.js                 |  8 ++-
 front/js/ui_components.js                 | 15 +++-
 front/php/server/db.php                   |  8 ++-
 server/app_state.py                       |  3 +-
 server/database.py                        |  5 ++
 test/backend/test_execution_queue.py      | 86 +++++++++++++++++++++++
 test/server/test_app_state_sse.py         | 64 +++++++++++++++++
 test/server/test_database_busy_timeout.py | 43 ++++++++++++
 10 files changed, 233 insertions(+), 9 deletions(-)
 create mode 100644 test/backend/test_execution_queue.py
 create mode 100644 test/server/test_app_state_sse.py
 create mode 100644 test/server/test_database_busy_timeout.py

diff --git a/CLAUDE.md b/CLAUDE.md
index 9b7729fe..bdd97412 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -89,5 +89,5 @@ Procedural/how-to knowledge (running tests, resetting the DB, devcontainer manag
 - No inline imports — everything at module top level.
 - Reuse `test/db_test_helpers.py` for DB mocks/fixtures in tests rather than redefining `DummyDB`/`make_db` locally.
 - Keep files under ~500 lines; split rather than grow.
-- Every Python function/method gets a succinct docstring describing its current use and behavior — one or two sentences, not a changelog of what changed or why (that belongs in the commit/PR, not the docstring).
+- Every Python function/method gets a succinct docstring describing its current use and behavior — one or two sentences, not a changelog of what changed or why (that belongs in the commit/PR, not the docstring). Same rule for JS: a JSDoc `/** ... */` block, not a plain `//` line above the function. Whenever you touch a function that only has a plain description comment (Python or JS), convert it to a proper docstring as part of that edit rather than leaving the old style next to new code.
 - Before adding a new key to `front/php/templates/language/en_us.json`, search it for an existing key with the same text/purpose and reuse it — prefer generic `Gen_*` keys over page-scoped names for genuinely generic UI text (e.g. `Gen_Prev`/`Gen_Next`, not `Presence_Page_Prev`). Only the English file needs a new key; other locales fall back to it automatically.
diff --git a/front/js/common.js b/front/js/common.js
index fb45fce6..b555fa3a 100755
--- a/front/js/common.js
+++ b/front/js/common.js
@@ -1098,12 +1098,16 @@ function hideSpinner() {
 
 
 // --------------------------------------------------------
-// Calls a backend function to add a front-end event to an execution queue
+/**
+ * Queue an "update_api" ad-hoc event so the backend refreshes just the
+ * given data sources (e.g. "devices,appevents") on its next tick.
+ * @param {string} apiEndpoints - Comma-separated data source names.
+ */
 function updateApi(apiEndpoints)
 {
 
   // value has to be in format event|param. e.g. run|ARPSCAN
-  action = `${getGuid()}|update_api|${apiEndpoints}`
+  action = `update_api|${apiEndpoints}`
 
   const { token: apiToken, apiBase: apiBaseUrl, authHeader } = getAuthContext();
   const url = `${apiBaseUrl}/logs/add-to-execution-queue`;
diff --git a/front/js/devices-table.js b/front/js/devices-table.js
index 8eb004a9..c46565d7 100644
--- a/front/js/devices-table.js
+++ b/front/js/devices-table.js
@@ -620,6 +620,12 @@ function initializeDatatable (status) {
 
 
 // -----------------------------------------------------------------------------
+/**
+ * Poll execution_queue.log for a pending "update_api|devices" entry and show
+ * a spinner while it's queued; reload the page once it clears if a reload
+ * was requested.
+ * @param {boolean} [needsReload=false] - Reload the page once the queue clears.
+ */
 function handleLoadingDialog(needsReload = false)
 {
   // console.log(`needsReload: ${needsReload}`);
@@ -630,7 +636,7 @@ function handleLoadingDialog(needsReload = false)
     {
       showSpinner("devices_old")
 
-      setTimeout(handleLoadingDialog(true), 1000);
+      setTimeout(() => handleLoadingDialog(true), 1000);
 
     } else if (needsReload)
     {
diff --git a/front/js/ui_components.js b/front/js/ui_components.js
index 77ee5493..80bc77a7 100755
--- a/front/js/ui_components.js
+++ b/front/js/ui_components.js
@@ -549,8 +549,14 @@ function goToDevice(mac, newtab = false) {
 
 
 // --------------------------------------------------------
-// Updating the execution queue in in modal pop-up
-function updateModalState() {
+/**
+ * Poll execution_queue.log into the ad-hoc-event modal until it's empty or a
+ * safety cap is hit (was: polled forever with no stop condition).
+ * @param {number} [elapsedMs=0] - Total time already spent polling.
+ */
+function updateModalState(elapsedMs = 0) {
+  const MAX_POLL_MS = 60000; // safety net - stop after 1 minute regardless
+
   setTimeout(function() {
       // Fetch the content from the log file using an AJAX request
       $.ajax({
@@ -560,7 +566,10 @@ function updateModalState() {
               // Update the content of the HTML element (e.g., a div with id 'logContent')
               $('#'+modalEventStatusId).html(data);
 
-              updateModalState();
+              if (data.trim() === '' || elapsedMs + 2000 >= MAX_POLL_MS) {
+                return; // queue drained (or safety net hit) - stop polling
+              }
+              updateModalState(elapsedMs + 2000);
           },
           error: function() {
               // Handle error, such as the file not being found
diff --git a/front/php/server/db.php b/front/php/server/db.php
index 2076c6fa..6f6d5a65 100755
--- a/front/php/server/db.php
+++ b/front/php/server/db.php
@@ -76,7 +76,13 @@ function SQLite3_connect($trytoreconnect = true, $retryCount = 0) {
         // Write unlock status to the locked file
         file_put_contents($DBFILE_LOCKED_FILE, '0');
 
-        return new SQLite3($DBFILE, SQLITE3_OPEN_READWRITE);
+        $conn = new SQLite3($DBFILE, SQLITE3_OPEN_READWRITE);
+        // Wait up to 5s for a lock before returning SQLITE_BUSY, matching the
+        // Python main-loop connection's PRAGMA busy_timeout - without this,
+        // any collision with a concurrent writer fails immediately instead
+        // of retrying internally.
+        $conn->busyTimeout(5000);
+        return $conn;
     } catch (Exception $exception) {
         // sqlite3 throws an exception when it is unable to connect
         global $db_locked;
diff --git a/server/app_state.py b/server/app_state.py
index ba1bef4e..1e9fd389 100755
--- a/server/app_state.py
+++ b/server/app_state.py
@@ -189,7 +189,8 @@ class app_state_class:
                     buildTimestamp=self.buildTimestamp,
                     last_scan_run=self.last_scan_run,
                     next_scan_time=self.next_scan_time,
-                    pause_until=self.pause_until
+                    pause_until=self.pause_until,
+                    pluginsStates=self.pluginsStates
                 )
             except Exception as e:
                 mylog("none", [f"[app_state] SSE broadcast: {e}"])
diff --git a/server/database.py b/server/database.py
index 44a089fc..ba66ef14 100755
--- a/server/database.py
+++ b/server/database.py
@@ -73,6 +73,11 @@ class DB:
             # The WAL journaling mode uses a write-ahead log instead of a
             # rollback journal to implement transactions.
             self.sql_connection.execute("pragma journal_mode=WAL;")
+            # Wait up to 5s for a lock before raising "database is locked",
+            # matching get_temp_db_connection()'s value - without this, any
+            # collision with a concurrent writer (e.g. a PHP request) fails
+            # immediately instead of retrying internally.
+            self.sql_connection.execute("PRAGMA busy_timeout=5000;")
             # When synchronous is NORMAL (1), the SQLite database engine will
             # still sync at the most critical moments,
             # but less often than in FULL mode.
diff --git a/test/backend/test_execution_queue.py b/test/backend/test_execution_queue.py
new file mode 100644
index 00000000..cbdb1d95
--- /dev/null
+++ b/test/backend/test_execution_queue.py
@@ -0,0 +1,86 @@
+"""
+Unit tests for the execution_queue.log action-string format and dispatch
+(server/models/user_events_queue_instance.py, plugin.check_and_run_user_event()).
+
+Covers the fix for updateApi() (front/js/common.js) prepending an extra
+client-side GUID onto its action string, which broke check_and_run_user_event()
+and finalize_event()'s shared "split('|')[2:4]" parsing - see
+.gemini/internal-docs/PRDs/execution-queue-fe-locking-fix.md, fix A.
+"""
+
+import os
+import sys
+import tempfile
+import shutil
+import unittest
+from types import SimpleNamespace
+from unittest.mock import patch
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server"))
+
+from models.user_events_queue_instance import UserEventsQueueInstance  # noqa: E402
+import plugin as plugin_module  # noqa: E402
+
+
+class TestExecutionQueueDispatch(unittest.TestCase):
+    """Isolates execution_queue.log to a temp dir so tests don't touch the
+    real logPath, and don't depend on plugin_manager's full __init__ (DB
+    settings cache, schedules, logger) - check_and_run_user_event() only
+    touches self.db/self.all_plugins, so a bare SimpleNamespace stands in."""
+
+    def setUp(self):
+        self._tmpdir = tempfile.mkdtemp()
+        self._patcher = patch("models.user_events_queue_instance.logPath", self._tmpdir)
+        self._patcher.start()
+
+    def tearDown(self):
+        self._patcher.stop()
+        shutil.rmtree(self._tmpdir, ignore_errors=True)
+
+    def _fake_manager(self):
+        return SimpleNamespace(db=None, all_plugins=[])
+
+    def test_check_and_run_user_event_dispatches_update_api(self):
+        """The fixed action format (no extra client GUID) must reach the
+        elif event == "update_api" branch and call update_api() with the
+        parsed params and is_ad_hoc_user_event=True. finalize_event("update_api")
+        is called inside the real update_api()'s try_write() (api.py:205-208),
+        not here - since update_api is mocked, the line is expected to still
+        be in the log (verified separately in test_finalize_event_removes_update_api_line)."""
+        q = UserEventsQueueInstance()
+        q.add_event("update_api|devices,appevents")
+
+        with patch("plugin.update_api") as mock_update_api:
+            plugin_module.plugin_manager.check_and_run_user_event(self._fake_manager())
+
+        mock_update_api.assert_called_once_with(None, [], False, ["devices", "appevents"], True)
+
+    def test_finalize_event_removes_update_api_line(self):
+        q = UserEventsQueueInstance()
+        q.add_event("update_api|devices,appevents")
+
+        removed = q.finalize_event("update_api")
+
+        self.assertTrue(removed)
+        self.assertEqual(q.read_log(), [])
+
+    def test_malformed_action_falls_through_to_unhandled_branch(self):
+        """Regression guard for the exact bug fixed: an action string with an
+        extra field before "update_api" (the old updateApi() format) must
+        NOT reach the update_api() call - it's misrouted into the "else"
+        unhandled-event branch instead. The line still gets removed (both
+        check_and_run_user_event() and finalize_event() parse it the same
+        wrong way and agree with each other), but the intended fast-refresh
+        never fires."""
+        q = UserEventsQueueInstance()
+        q.add_event("11111111-1111-1111-1111-111111111111|update_api|devices,appevents")
+
+        with patch("plugin.update_api") as mock_update_api:
+            plugin_module.plugin_manager.check_and_run_user_event(self._fake_manager())
+
+        mock_update_api.assert_not_called()
+        self.assertEqual(q.read_log(), [])
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/test/server/test_app_state_sse.py b/test/server/test_app_state_sse.py
new file mode 100644
index 00000000..ce4ec053
--- /dev/null
+++ b/test/server/test_app_state_sse.py
@@ -0,0 +1,64 @@
+"""
+Regression guard for server/app_state.py's updateState()/broadcast_state_update()
+call - pluginsStates must reach the SSE broadcast payload, not just the
+persisted app_state.json. See
+.gemini/internal-docs/PRDs/to_review/execution-queue-fe-locking-fix.md's
+post-implementation addendum: the original broadcast_state_update() call
+never passed pluginsStates, so front/js/ui_components.js's watchPluginState()
+(fix C1) waited on an SSE event that could never arrive.
+"""
+
+import os
+import sys
+import tempfile
+import shutil
+import unittest
+from unittest.mock import patch
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server"))
+
+import app_state  # noqa: E402
+
+
+class TestAppStateSSEBroadcast(unittest.TestCase):
+    def setUp(self):
+        self._tmpdir = tempfile.mkdtemp()
+        self._patchers = [
+            patch("app_state.apiPath", self._tmpdir + os.sep),
+            patch("app_state.checkNewVersion", lambda *a, **k: False),
+        ]
+        for p in self._patchers:
+            p.start()
+
+    def tearDown(self):
+        for p in self._patchers:
+            p.stop()
+        shutil.rmtree(self._tmpdir, ignore_errors=True)
+
+    def test_update_state_broadcasts_plugins_states(self):
+        with patch("app_state.broadcast_state_update") as mock_broadcast:
+            app_state.updateState(
+                pluginsStates={"INTRSPD": {"stateUpdated": "2026-09-14 12:00:00", "totalObjects": 1}}
+            )
+
+        mock_broadcast.assert_called_once()
+        _, kwargs = mock_broadcast.call_args
+        self.assertIn("pluginsStates", kwargs)
+        self.assertEqual(kwargs["pluginsStates"]["INTRSPD"]["totalObjects"], 1)
+
+    def test_plugins_states_persist_and_merge_across_calls(self):
+        """updateState() merges into the existing pluginsStates dict rather
+        than replacing it - a second plugin's update must not drop the
+        first's entry, and each broadcast must carry the full merged dict."""
+        with patch("app_state.broadcast_state_update") as mock_broadcast:
+            app_state.updateState(pluginsStates={"PLUGINA": {"stateUpdated": "2026-09-14 12:00:00"}})
+            app_state.updateState(pluginsStates={"PLUGINB": {"stateUpdated": "2026-09-14 12:00:05"}})
+
+        self.assertEqual(mock_broadcast.call_count, 2)
+        _, last_kwargs = mock_broadcast.call_args
+        self.assertIn("PLUGINA", last_kwargs["pluginsStates"])
+        self.assertIn("PLUGINB", last_kwargs["pluginsStates"])
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/test/server/test_database_busy_timeout.py b/test/server/test_database_busy_timeout.py
new file mode 100644
index 00000000..acb1dbe3
--- /dev/null
+++ b/test/server/test_database_busy_timeout.py
@@ -0,0 +1,43 @@
+"""
+Regression guard for the main-loop SQLite connection's busy_timeout
+(server/database.py's DB.open()) - see
+.gemini/internal-docs/PRDs/execution-queue-fe-locking-fix.md, fix D.
+
+Without PRAGMA busy_timeout, a collision with a concurrent writer (e.g. a
+PHP request) fails immediately with "database is locked" instead of
+retrying internally for a bounded window.
+"""
+
+import os
+import sys
+import tempfile
+import shutil
+import unittest
+from unittest.mock import patch
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server"))
+
+from database import DB  # noqa: E402
+
+
+class TestDatabaseBusyTimeout(unittest.TestCase):
+    def setUp(self):
+        self._tmpdir = tempfile.mkdtemp()
+        self._db_path = os.path.join(self._tmpdir, "test_app.db")
+
+    def tearDown(self):
+        shutil.rmtree(self._tmpdir, ignore_errors=True)
+
+    def test_open_sets_busy_timeout_5000ms(self):
+        db = DB()
+        with patch("database.fullDbPath", self._db_path):
+            db.open()
+        try:
+            value = db.sql_connection.execute("PRAGMA busy_timeout;").fetchone()[0]
+            self.assertEqual(value, 5000)
+        finally:
+            db.sql_connection.close()
+
+
+if __name__ == "__main__":
+    unittest.main()

From e237b926571d841c2c4d8507593949d314176d7b Mon Sep 17 00:00:00 2001
From: jokob-sk <jokob.sk@gmail.com>
Date: Tue, 15 Sep 2026 08:46:44 +1000
Subject: [PATCH 2/2] BE+FE: plugin view fixes, skills, new core method and
 tests

---
 .claude/skills/plugin-review/SKILL.md   | 34 +++++++++++++
 .gemini/skills/plugin-review/SKILL.md   | 34 +++++++++++++
 .gemini/skills/skills-index/SKILL.md    |  1 +
 .github/skills/plugin-review/SKILL.md   | 34 +++++++++++++
 .github/skills/skills-overview/SKILL.md |  1 +
 front/pluginsCore.php                   | 47 ++++++++++++++----
 scripts/check_skill_pairs.py            |  1 +
 server/models/device_instance.py        |  7 +++
 test/backend/test_device_instance.py    | 64 +++++++++++++++++++++++++
 9 files changed, 214 insertions(+), 9 deletions(-)
 create mode 100644 .claude/skills/plugin-review/SKILL.md
 create mode 100644 .gemini/skills/plugin-review/SKILL.md
 create mode 100644 .github/skills/plugin-review/SKILL.md
 create mode 100644 test/backend/test_device_instance.py

diff --git a/.claude/skills/plugin-review/SKILL.md b/.claude/skills/plugin-review/SKILL.md
new file mode 100644
index 00000000..64c2a02e
--- /dev/null
+++ b/.claude/skills/plugin-review/SKILL.md
@@ -0,0 +1,34 @@
+---
+name: plugin-review
+description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers the one check not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method - plus a worked real-PR example.
+---
+
+# Plugin Review
+
+## 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.
+
+## The check this skill adds: no raw SQL in a plugin script
+
+Plugin scripts write their results to `RESULT_FILE` via `plugin_helper.Plugin_Objects` — the framework inserts those rows into the DB. A plugin that also runs its own `SELECT`/`INSERT`/`UPDATE` (via `sqlite3` directly or `database.get_temp_db_connection()`) is bypassing that contract, usually to read existing data before deciding what to write.
+
+**Default for a new/contributed plugin: use an existing model method (`server/models/*.py`), or add one, instead of a raw SQL string in the plugin.** Not GraphQL — GraphQL (`/graphql`) is the frontend-facing API; no plugin in this codebase reaches it, and doing so would mean an HTTP round-trip (with an API token) from a subprocess that already has direct `server/` import access. Every plugin that currently touches the DB imports `server/` modules directly, matching how the rest of the backend is layered (`CLAUDE.md`: "Never query the DB from elsewhere — go through a model or `db_helper.py`" — this is that same rule, just also applying to plugins).
+
+**Known exception — 5 existing core/infrastructure plugins legitimately use `get_temp_db_connection()` with raw SQL:** `db_cleanup` (retention `DELETE`s + `REINDEX` — no model method fits a multi-table retention sweep), `heartbeat`, `vendor_update`, `csv_backup`, `sync`. These predate this rule and do maintenance/schema-level work (bulk retention, `PRAGMA table_info`, full-table export) that doesn't map to a single-row model method. Don't treat their existence as precedent for a *new* plugin's simple lookup query — check whether an existing method already covers the new plugin's actual need first (it usually does, or is a one-line addition).
+
+## Review flow for a raw SQL query in a plugin
+
+1. **Does an existing model method already do this?** Check the relevant `server/models/*_instance.py` file (`DeviceInstance`, `EventInstance`, `PluginObjectInstance`, etc.) before assuming one needs to be added.
+2. **If not, is it worth adding one** (`server/models/device_instance.py` etc.), or is this genuinely a one-off maintenance/schema query that belongs in the core-plugin exception list above?
+3. **Check the collation the query relies on** against the column's actual schema (`server/db/schema/app.sql`) rather than assuming — `devMac`/`eveMac`/`sesMac`/`scanMac`/`devParentMAC` are declared `COLLATE NOCASE` at the column level, so an explicit `COLLATE NOCASE` against one of them in a new query is redundant (harmless, but a sign the author didn't check). `devName` has **no** column-level collation — an explicit `COLLATE NOCASE` there is genuinely necessary if case-insensitive name matching is intended, not a mistake.
+4. **Parameterization** — `?` placeholders, never string-formatted values into the query (this part is usually already fine; flag it if not).
+
+## Worked example: PR #1788 (DOCKERDISC plugin)
+
+Two raw queries in `server/plugins/dockerdisc/script.py`:
+
+- `lookup_device_mac()`: `SELECT 1 FROM Devices WHERE devMac = ? COLLATE NOCASE LIMIT 1` — an existence check. `DeviceInstance.getByMac(mac)` already does this exact lookup (`server/models/device_instance.py:102-105`); its `COLLATE NOCASE` is redundant since `devMac` already carries that collation at the column level. **Fix: `DeviceInstance().getByMac(mac) is not None`, delete the raw SQL.**
+- `resolve_host_mac()`: `SELECT devMac FROM Devices WHERE devName = ? COLLATE NOCASE` — a name lookup with real 0/1/many-match handling (falls back to a manually-configured MAC on ambiguity or no match). No existing method covers this. `devName` has no column-level collation, so the explicit `COLLATE NOCASE` here is correct, not redundant. **Fix: add `DeviceInstance.getAllByName(name)` returning every match** (not just one — the plugin's own ambiguity detection needs the full set), and have the plugin call that instead.
+
+This is the shape of the fix in general: an existence/single-row check usually already has a model method; a query with plugin-specific result handling (ambiguity, filtering) usually needs a small new method added rather than a workaround in the plugin itself.
diff --git a/.gemini/skills/plugin-review/SKILL.md b/.gemini/skills/plugin-review/SKILL.md
new file mode 100644
index 00000000..64c2a02e
--- /dev/null
+++ b/.gemini/skills/plugin-review/SKILL.md
@@ -0,0 +1,34 @@
+---
+name: plugin-review
+description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers the one check not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method - plus a worked real-PR example.
+---
+
+# Plugin Review
+
+## 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.
+
+## The check this skill adds: no raw SQL in a plugin script
+
+Plugin scripts write their results to `RESULT_FILE` via `plugin_helper.Plugin_Objects` — the framework inserts those rows into the DB. A plugin that also runs its own `SELECT`/`INSERT`/`UPDATE` (via `sqlite3` directly or `database.get_temp_db_connection()`) is bypassing that contract, usually to read existing data before deciding what to write.
+
+**Default for a new/contributed plugin: use an existing model method (`server/models/*.py`), or add one, instead of a raw SQL string in the plugin.** Not GraphQL — GraphQL (`/graphql`) is the frontend-facing API; no plugin in this codebase reaches it, and doing so would mean an HTTP round-trip (with an API token) from a subprocess that already has direct `server/` import access. Every plugin that currently touches the DB imports `server/` modules directly, matching how the rest of the backend is layered (`CLAUDE.md`: "Never query the DB from elsewhere — go through a model or `db_helper.py`" — this is that same rule, just also applying to plugins).
+
+**Known exception — 5 existing core/infrastructure plugins legitimately use `get_temp_db_connection()` with raw SQL:** `db_cleanup` (retention `DELETE`s + `REINDEX` — no model method fits a multi-table retention sweep), `heartbeat`, `vendor_update`, `csv_backup`, `sync`. These predate this rule and do maintenance/schema-level work (bulk retention, `PRAGMA table_info`, full-table export) that doesn't map to a single-row model method. Don't treat their existence as precedent for a *new* plugin's simple lookup query — check whether an existing method already covers the new plugin's actual need first (it usually does, or is a one-line addition).
+
+## Review flow for a raw SQL query in a plugin
+
+1. **Does an existing model method already do this?** Check the relevant `server/models/*_instance.py` file (`DeviceInstance`, `EventInstance`, `PluginObjectInstance`, etc.) before assuming one needs to be added.
+2. **If not, is it worth adding one** (`server/models/device_instance.py` etc.), or is this genuinely a one-off maintenance/schema query that belongs in the core-plugin exception list above?
+3. **Check the collation the query relies on** against the column's actual schema (`server/db/schema/app.sql`) rather than assuming — `devMac`/`eveMac`/`sesMac`/`scanMac`/`devParentMAC` are declared `COLLATE NOCASE` at the column level, so an explicit `COLLATE NOCASE` against one of them in a new query is redundant (harmless, but a sign the author didn't check). `devName` has **no** column-level collation — an explicit `COLLATE NOCASE` there is genuinely necessary if case-insensitive name matching is intended, not a mistake.
+4. **Parameterization** — `?` placeholders, never string-formatted values into the query (this part is usually already fine; flag it if not).
+
+## Worked example: PR #1788 (DOCKERDISC plugin)
+
+Two raw queries in `server/plugins/dockerdisc/script.py`:
+
+- `lookup_device_mac()`: `SELECT 1 FROM Devices WHERE devMac = ? COLLATE NOCASE LIMIT 1` — an existence check. `DeviceInstance.getByMac(mac)` already does this exact lookup (`server/models/device_instance.py:102-105`); its `COLLATE NOCASE` is redundant since `devMac` already carries that collation at the column level. **Fix: `DeviceInstance().getByMac(mac) is not None`, delete the raw SQL.**
+- `resolve_host_mac()`: `SELECT devMac FROM Devices WHERE devName = ? COLLATE NOCASE` — a name lookup with real 0/1/many-match handling (falls back to a manually-configured MAC on ambiguity or no match). No existing method covers this. `devName` has no column-level collation, so the explicit `COLLATE NOCASE` here is correct, not redundant. **Fix: add `DeviceInstance.getAllByName(name)` returning every match** (not just one — the plugin's own ambiguity detection needs the full set), and have the plugin call that instead.
+
+This is the shape of the fix in general: an existence/single-row check usually already has a model method; a query with plugin-specific result handling (ambiguity, filtering) usually needs a small new method added rather than a workaround in the plugin itself.
diff --git a/.gemini/skills/skills-index/SKILL.md b/.gemini/skills/skills-index/SKILL.md
index bd47c8b5..56e03998 100644
--- a/.gemini/skills/skills-index/SKILL.md
+++ b/.gemini/skills/skills-index/SKILL.md
@@ -33,6 +33,7 @@ Skills with the same purpose exist in more than one, sometimes under different n
 | PRD writing | `prd-writing` | `prd-writing` | `prd-writing` | Methodology for writing a design doc: challenge the idea, verify every claim against actual code, trace every downstream consumer of a new mechanism, evaluate performance impact against the real schema/indexes, record rejected alternatives and open-issue decisions explicitly, final-check pass before done. |
 | UX/frontend design | `ux-design-patterns` | `ux-design-patterns` | `ux-design-patterns` | Don't invent new UX behavior/visual patterns unless a PRD calls for it - search `front/` for an existing pattern first and reuse it. Priority order for design tradeoffs when several options are reasonable: existing behavior > intuitiveness > information density > usability > utility > uniqueness > industry practices > generic UI. |
 | Skill hygiene | `skill-hygiene` | `skill-hygiene` | `skill-hygiene` | Read before writing/editing any SKILL.md, or any research/audit doc in `.gemini/internal-docs/research/`. Two standing rules: state current behavior only (no "Correction:", no "as of <date>", no "caught in review" narration - that trail belongs in PRDs), and prefer plain, short wording. Includes the grep sweep to run before calling a doc clean. |
+| Plugin review | `plugin-review` | `plugin-review` | `plugin-review` | Reviewer-facing complement to Plugin dev above: no raw SQL in a plugin script - use an existing/new `server/models/*.py` method, not GraphQL (which no plugin reaches). Named exception list for 5 pre-existing core/infra plugins with legitimate direct DB access, a collation-checking step, and a worked example from a real PR (#1788, DOCKERDISC). |
 
 ---
 
diff --git a/.github/skills/plugin-review/SKILL.md b/.github/skills/plugin-review/SKILL.md
new file mode 100644
index 00000000..e10927e8
--- /dev/null
+++ b/.github/skills/plugin-review/SKILL.md
@@ -0,0 +1,34 @@
+---
+name: netalertx-plugin-review
+description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers the one check not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method - plus a worked real-PR example.
+---
+
+# Plugin Review
+
+## 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.
+
+## The check this skill adds: no raw SQL in a plugin script
+
+Plugin scripts write their results to `RESULT_FILE` via `plugin_helper.Plugin_Objects` — the framework inserts those rows into the DB. A plugin that also runs its own `SELECT`/`INSERT`/`UPDATE` (via `sqlite3` directly or `database.get_temp_db_connection()`) is bypassing that contract, usually to read existing data before deciding what to write.
+
+**Default for a new/contributed plugin: use an existing model method (`server/models/*.py`), or add one, instead of a raw SQL string in the plugin.** Not GraphQL — GraphQL (`/graphql`) is the frontend-facing API; no plugin in this codebase reaches it, and doing so would mean an HTTP round-trip (with an API token) from a subprocess that already has direct `server/` import access. Every plugin that currently touches the DB imports `server/` modules directly, matching how the rest of the backend is layered (`CLAUDE.md`: "Never query the DB from elsewhere — go through a model or `db_helper.py`" — this is that same rule, just also applying to plugins).
+
+**Known exception — 5 existing core/infrastructure plugins legitimately use `get_temp_db_connection()` with raw SQL:** `db_cleanup` (retention `DELETE`s + `REINDEX` — no model method fits a multi-table retention sweep), `heartbeat`, `vendor_update`, `csv_backup`, `sync`. These predate this rule and do maintenance/schema-level work (bulk retention, `PRAGMA table_info`, full-table export) that doesn't map to a single-row model method. Don't treat their existence as precedent for a *new* plugin's simple lookup query — check whether an existing method already covers the new plugin's actual need first (it usually does, or is a one-line addition).
+
+## Review flow for a raw SQL query in a plugin
+
+1. **Does an existing model method already do this?** Check the relevant `server/models/*_instance.py` file (`DeviceInstance`, `EventInstance`, `PluginObjectInstance`, etc.) before assuming one needs to be added.
+2. **If not, is it worth adding one** (`server/models/device_instance.py` etc.), or is this genuinely a one-off maintenance/schema query that belongs in the core-plugin exception list above?
+3. **Check the collation the query relies on** against the column's actual schema (`server/db/schema/app.sql`) rather than assuming — `devMac`/`eveMac`/`sesMac`/`scanMac`/`devParentMAC` are declared `COLLATE NOCASE` at the column level, so an explicit `COLLATE NOCASE` against one of them in a new query is redundant (harmless, but a sign the author didn't check). `devName` has **no** column-level collation — an explicit `COLLATE NOCASE` there is genuinely necessary if case-insensitive name matching is intended, not a mistake.
+4. **Parameterization** — `?` placeholders, never string-formatted values into the query (this part is usually already fine; flag it if not).
+
+## Worked example: PR #1788 (DOCKERDISC plugin)
+
+Two raw queries in `server/plugins/dockerdisc/script.py`:
+
+- `lookup_device_mac()`: `SELECT 1 FROM Devices WHERE devMac = ? COLLATE NOCASE LIMIT 1` — an existence check. `DeviceInstance.getByMac(mac)` already does this exact lookup (`server/models/device_instance.py:102-105`); its `COLLATE NOCASE` is redundant since `devMac` already carries that collation at the column level. **Fix: `DeviceInstance().getByMac(mac) is not None`, delete the raw SQL.**
+- `resolve_host_mac()`: `SELECT devMac FROM Devices WHERE devName = ? COLLATE NOCASE` — a name lookup with real 0/1/many-match handling (falls back to a manually-configured MAC on ambiguity or no match). No existing method covers this. `devName` has no column-level collation, so the explicit `COLLATE NOCASE` here is correct, not redundant. **Fix: add `DeviceInstance.getAllByName(name)` returning every match** (not just one — the plugin's own ambiguity detection needs the full set), and have the plugin call that instead.
+
+This is the shape of the fix in general: an existence/single-row check usually already has a model method; a query with plugin-specific result handling (ambiguity, filtering) usually needs a small new method added rather than a workaround in the plugin itself.
diff --git a/.github/skills/skills-overview/SKILL.md b/.github/skills/skills-overview/SKILL.md
index 9a94b293..40607658 100644
--- a/.github/skills/skills-overview/SKILL.md
+++ b/.github/skills/skills-overview/SKILL.md
@@ -33,6 +33,7 @@ Skills with the same purpose exist in more than one, sometimes under different n
 | PRD writing | `prd-writing` | `prd-writing` | `prd-writing` | Methodology for writing a design doc: challenge the idea, verify every claim against actual code, trace every downstream consumer of a new mechanism, evaluate performance impact against the real schema/indexes, record rejected alternatives and open-issue decisions explicitly, final-check pass before done. |
 | UX/frontend design | `ux-design-patterns` | `ux-design-patterns` | `ux-design-patterns` | Don't invent new UX behavior/visual patterns unless a PRD calls for it - search `front/` for an existing pattern first and reuse it. Priority order for design tradeoffs when several options are reasonable: existing behavior > intuitiveness > information density > usability > utility > uniqueness > industry practices > generic UI. |
 | Skill hygiene | `skill-hygiene` | `skill-hygiene` | `skill-hygiene` | Read before writing/editing any SKILL.md, or any research/audit doc in `.gemini/internal-docs/research/`. Two standing rules: state current behavior only (no "Correction:", no "as of <date>", no "caught in review" narration - that trail belongs in PRDs), and prefer plain, short wording. Includes the grep sweep to run before calling a doc clean. |
+| Plugin review | `plugin-review` | `plugin-review` | `plugin-review` | Reviewer-facing complement to Plugin dev above: no raw SQL in a plugin script - use an existing/new `server/models/*.py` method, not GraphQL (which no plugin reaches). Named exception list for 5 pre-existing core/infra plugins with legitimate direct DB access, a collation-checking step, and a worked example from a real PR (#1788, DOCKERDISC). |
 
 ---
 
diff --git a/front/pluginsCore.php b/front/pluginsCore.php
index 5b4fba15..869a963d 100755
--- a/front/pluginsCore.php
+++ b/front/pluginsCore.php
@@ -484,6 +484,11 @@ function autoHideEmptyTabs(counts, prefixes) {
   });
 }
 
+/**
+ * Build the plugin tab headers/panes for every show_ui plugin with a
+ * non-zero object/event/history count, and wire up (re-)initialization of
+ * each one's DataTable(s) via shown.bs.tab.
+ */
 function generateTabs() {
 
   // Reset the tabs by clearing previous headers and content
@@ -515,7 +520,10 @@ function generateTabs() {
 
   // Now that ALL DOM elements exist (both <a> headers and tab panes),
   // wire up DataTable initialization: immediate for the active tab,
-  // deferred via shown.bs.tab for the rest.
+  // on every subsequent shown.bs.tab for the rest - not just the first,
+  // so revisiting a plugin tab refreshes its active sub-table instead of
+  // leaving it stuck on whatever its one-time first fetch returned
+  // (namespaced + off() first so repeat calls don't stack duplicate handlers).
   let firstVisible = true;
   visiblePlugins.forEach(pluginObj => {
     const prefix = pluginObj.unique_prefix;
@@ -524,9 +532,11 @@ function generateTabs() {
       initializeDataTables(prefix, colDefinitions, pluginObj);
       firstVisible = false;
     } else {
-      $(`a[href="#${prefix}"]`).one('shown.bs.tab', function() {
-        initializeDataTables(prefix, colDefinitions, pluginObj);
-      });
+      $(`a[href="#${prefix}"]`)
+        .off('shown.bs.tab.pluginsCore')
+        .on('shown.bs.tab.pluginsCore', function() {
+          initializeDataTables(prefix, colDefinitions, pluginObj);
+        });
     }
   });
 
@@ -651,6 +661,14 @@ function generateDataTable(prefix, tableType, colDefinitions) {
   `;
 }
 
+/**
+ * Build (or, if already built, refresh) the Objects/Events/History
+ * DataTables for one plugin - the active sub-tab immediately, the other two
+ * on their own shown.bs.tab. Safe to call more than once per prefix.
+ * @param {string} prefix - The plugin's unique_prefix.
+ * @param {object[]} colDefinitions - This plugin's visible database_column_definitions.
+ * @param {object} pluginObj - The full plugin definition from plugins.json.
+ */
 function initializeDataTables(prefix, colDefinitions, pluginObj) {
   const mac        = $("#txtMacFilter").val();
   const foreignKey = (mac && mac !== "--") ? mac : null;
@@ -661,9 +679,16 @@ function initializeDataTables(prefix, colDefinitions, pluginObj) {
     { tableId: `historyTable_${prefix}`, gqlField: 'pluginsHistory', countId: `histCount_${prefix}`, badgeId: null },
   ];
 
+  /**
+   * Build tableId's DataTable on first call; on any later call, refresh its
+   * data in place instead of no-op'ing (the whole point of this fix).
+   */
   function buildDT(tableId, gqlField, countId, badgeId) {
     if ($.fn.DataTable.isDataTable(`#${tableId}`)) {
-      return; // already initialized
+      // Already built - refresh in place (keep current page/sort) instead of
+      // leaving it stuck on whatever its first-ever fetch returned.
+      $(`#${tableId}`).DataTable().ajax.reload(null, false);
+      return;
     }
     const skelId = `#skel-${tableId.replace('Table_', 'Target_')}`;
     $(`#${tableId}`).DataTable({
@@ -719,10 +744,14 @@ function initializeDataTables(prefix, colDefinitions, pluginObj) {
       // This sub-tab is the currently active one — initialize immediately
       buildDT(cfg.tableId, cfg.gqlField, cfg.countId, cfg.badgeId);
     } else if ($subPane.closest('.tab-pane').length) {
-      // Defer until shown
-      $(`a[href="${href}"]`).one('shown.bs.tab', function() {
-        buildDT(cfg.tableId, cfg.gqlField, cfg.countId, cfg.badgeId);
-      });
+      // Build on first shown, refresh (via buildDT's reload branch) on every
+      // one after - namespaced + off() first since initializeDataTables()
+      // itself can now run more than once (see generateTabs()).
+      $(`a[href="${href}"]`)
+        .off('shown.bs.tab.pluginsCore')
+        .on('shown.bs.tab.pluginsCore', function() {
+          buildDT(cfg.tableId, cfg.gqlField, cfg.countId, cfg.badgeId);
+        });
     }
   });
 }
diff --git a/scripts/check_skill_pairs.py b/scripts/check_skill_pairs.py
index 85107328..6cf2ef98 100644
--- a/scripts/check_skill_pairs.py
+++ b/scripts/check_skill_pairs.py
@@ -33,6 +33,7 @@ GROUPS = [
     [".gemini/skills/prd-writing/SKILL.md", ".github/skills/prd-writing/SKILL.md", ".claude/skills/prd-writing/SKILL.md"],
     [".gemini/skills/ux-design-patterns/SKILL.md", ".github/skills/ux-design-patterns/SKILL.md", ".claude/skills/ux-design-patterns/SKILL.md"],
     [".gemini/skills/skill-hygiene/SKILL.md", ".github/skills/skill-hygiene/SKILL.md", ".claude/skills/skill-hygiene/SKILL.md"],
+    [".gemini/skills/plugin-review/SKILL.md", ".github/skills/plugin-review/SKILL.md", ".claude/skills/plugin-review/SKILL.md"],
     [".gemini/skills/settings/SKILL.md", ".github/skills/settings-management/SKILL.md"],
     [".gemini/skills/mcp-activation/SKILL.md", ".github/skills/mcp-activation/SKILL.md"],
     [".gemini/skills/project-navigation/SKILL.md", ".github/skills/project-navigation/SKILL.md"],
diff --git a/server/models/device_instance.py b/server/models/device_instance.py
index d070868c..415e2e25 100755
--- a/server/models/device_instance.py
+++ b/server/models/device_instance.py
@@ -115,6 +115,13 @@ class DeviceInstance:
             SELECT * FROM Devices WHERE devLastIP = ?
         """, (ip,))
 
+    def getAllByName(self, name):
+        """Return every device whose devName matches (case-insensitive) - devName has
+        no column-level collation, so COLLATE NOCASE is explicit here, unlike devMac."""
+        return self._fetchall("""
+            SELECT * FROM Devices WHERE devName = ? COLLATE NOCASE
+        """, (name,))
+
     def queryByConditions(self, conditions):
         """Query Devices using a list of condition dicts.
 
diff --git a/test/backend/test_device_instance.py b/test/backend/test_device_instance.py
new file mode 100644
index 00000000..8ad4cf88
--- /dev/null
+++ b/test/backend/test_device_instance.py
@@ -0,0 +1,64 @@
+"""
+Unit tests for server/models/device_instance.py's DeviceInstance model methods.
+
+Covers:
+  - DeviceInstance.getAllByName()
+"""
+
+import sys
+import os
+import unittest
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server"))
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
+
+from db_test_helpers import make_db, make_device_dict, insert_device_from_dict
+
+
+class TestGetAllByName(unittest.TestCase):
+    """devName has no column-level collation (unlike devMac), so getAllByName()
+    must apply COLLATE NOCASE itself, and must return every match rather than
+    just one - callers (e.g. the dockerdisc plugin's resolve_host_mac()) rely
+    on the full set to detect an ambiguous (multiple-match) name."""
+
+    def setUp(self):
+        self.conn = make_db()
+        devices = [
+            make_device_dict("aa:bb:cc:dd:ee:01", devName="docker-host-1"),
+            make_device_dict("aa:bb:cc:dd:ee:02", devName="Docker-Host-1"),
+            make_device_dict("aa:bb:cc:dd:ee:03", devName="other-host"),
+        ]
+        for d in devices:
+            insert_device_from_dict(self.conn, d)
+        self.conn.commit()
+
+    def _instance(self):
+        from models.device_instance import DeviceInstance
+        inst = DeviceInstance()
+
+        def _fetchall(q, p=()):
+            rows = self.conn.execute(q, p).fetchall()
+            return [dict(r) for r in rows]
+        inst._fetchall = _fetchall
+        return inst
+
+    def test_case_insensitive_match_returns_all_ambiguous_rows(self):
+        inst = self._instance()
+        results = inst.getAllByName("docker-host-1")
+        macs = {r["devMac"] for r in results}
+        self.assertEqual(macs, {"aa:bb:cc:dd:ee:01", "aa:bb:cc:dd:ee:02"})
+
+    def test_case_insensitive_match_different_case_query(self):
+        inst = self._instance()
+        results = inst.getAllByName("OTHER-HOST")
+        self.assertEqual(len(results), 1)
+        self.assertEqual(results[0]["devMac"], "aa:bb:cc:dd:ee:03")
+
+    def test_no_match_returns_empty_list(self):
+        inst = self._instance()
+        results = inst.getAllByName("does-not-exist")
+        self.assertEqual(results, [])
+
+
+if __name__ == "__main__":
+    unittest.main()