From c0718d86188691231daeed07a5c575787d69b5ef Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 08:38:19 +0000 Subject: [PATCH 1/5] Initial plan From 283bea21c98f35d6e0ba4c672a8ff1fa069dfe85 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 08:44:00 +0000 Subject: [PATCH 2/5] fix: NIC child presence no longer forces a directly-detected parent offline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a parent device has NIC children, update_devPresentLastScan_based_on_nics previously replaced the parent's devPresentLastScan unconditionally with the NIC-derived value. This discarded any genuine direct detection of the parent: if the parent was found by ARP/save_own_device (present=1) but its NIC child was absent (present=0), the NIC step forced the parent back to 0. The next scan re-detected the parent → Connected event → NIC forced it down again, producing an endless one-directional Connected event stream. Fix: use max(original, nic_derived) so NIC children can only raise a parent's presence (bring an undetected parent online), never lower it when the parent itself was directly detected this cycle. Adds test/scan/test_nic_presence.py covering the exact regression scenario and surrounding cases (raise, no-NIC unchanged, req_all modes). Fixes #1736 Co-authored-by: jokob-sk <96159884+jokob-sk@users.noreply.github.com> --- server/scan/device_handling.py | 7 +- test/scan/test_nic_presence.py | 167 +++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 test/scan/test_nic_presence.py diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 523ad5152..53dc6152f 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -1270,9 +1270,12 @@ def update_devPresentLastScan_based_on_nics(db): if nics: nic_statuses = [nic.get("devPresentLastScan") == 1 for nic in nics] if req_all: - new_present = int(all(nic_statuses)) + nic_derived = int(all(nic_statuses)) else: - new_present = int(any(nic_statuses)) + nic_derived = int(any(nic_statuses)) + # NIC children can only raise a parent's presence, never lower it + # when the parent itself was directly detected as present this scan. + new_present = max(original, nic_derived) # Only add update if changed if original != new_present: diff --git a/test/scan/test_nic_presence.py b/test/scan/test_nic_presence.py new file mode 100644 index 000000000..be82e59e3 --- /dev/null +++ b/test/scan/test_nic_presence.py @@ -0,0 +1,167 @@ +"""Tests for update_devPresentLastScan_based_on_nics. + +Regression coverage for the bug where a parent device with a 'nic' child +relationship had its own directly-detected presence overwritten by the NIC +child's absence, producing an endless one-directional Connected event stream. +""" + +import sqlite3 + +import pytest + +from server.scan import device_handling + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_db(rows): + """Return a DummyDB-compatible object populated with the given Devices rows.""" + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + cur = conn.cursor() + cur.execute( + """ + CREATE TABLE Devices ( + devMac TEXT PRIMARY KEY, + devPresentLastScan INTEGER DEFAULT 0, + devParentMAC TEXT, + devParentRelType TEXT DEFAULT '', + devReqNicsOnline INTEGER DEFAULT 0 + ) + """ + ) + cur.executemany( + """ + INSERT INTO Devices (devMac, devPresentLastScan, devParentMAC, + devParentRelType, devReqNicsOnline) + VALUES (:mac, :present, :parent_mac, :rel_type, :req_all) + """, + rows, + ) + conn.commit() + + class DummyDB: + def __init__(self, connection): + self.sql = connection.cursor() + self._conn = connection + + def commitDB(self): + self._conn.commit() + + db = DummyDB(conn) + # Re-use conn cursor for later reads + db._raw_conn = conn + return db + + +def _present(db, mac): + row = db._raw_conn.execute( + "SELECT devPresentLastScan FROM Devices WHERE devMac = ?", (mac,) + ).fetchone() + return row[0] + + +# --------------------------------------------------------------------------- +# Core bug regression: parent directly detected (present=1) + absent NIC child +# --------------------------------------------------------------------------- + +class TestNicChildDoesNotForcePresentParentDown: + """Parent was directly detected this scan; absent NIC must NOT override that.""" + + def test_any_mode_absent_nic_does_not_clear_present_parent(self): + """Bug: req_all=0, parent present=1, nic present=0 → parent must stay 1.""" + db = _make_db([ + {"mac": "AA:AA:AA:AA:AA:01", "present": 1, + "parent_mac": "", "rel_type": "", "req_all": 0}, + {"mac": "BB:BB:BB:BB:BB:01", "present": 0, + "parent_mac": "AA:AA:AA:AA:AA:01", "rel_type": "nic", "req_all": 0}, + ]) + device_handling.update_devPresentLastScan_based_on_nics(db) + assert _present(db, "AA:AA:AA:AA:AA:01") == 1, ( + "Parent directly detected as present must not be forced offline " + "by an absent NIC child." + ) + + def test_req_all_mode_absent_nic_does_not_clear_present_parent(self): + """Bug: req_all=1, parent present=1, nic present=0 → parent must stay 1.""" + db = _make_db([ + {"mac": "AA:AA:AA:AA:AA:02", "present": 1, + "parent_mac": "", "rel_type": "", "req_all": 1}, + {"mac": "BB:BB:BB:BB:BB:02", "present": 0, + "parent_mac": "AA:AA:AA:AA:AA:02", "rel_type": "nic", "req_all": 0}, + ]) + device_handling.update_devPresentLastScan_based_on_nics(db) + assert _present(db, "AA:AA:AA:AA:AA:02") == 1 + + +# --------------------------------------------------------------------------- +# NIC can still raise an undetected parent (original=0) +# --------------------------------------------------------------------------- + +class TestNicRaisesAbsentParent: + """NIC children should be able to mark a parent present when it wasn't seen directly.""" + + def test_any_mode_online_nic_raises_absent_parent(self): + db = _make_db([ + {"mac": "AA:AA:AA:AA:AA:03", "present": 0, + "parent_mac": "", "rel_type": "", "req_all": 0}, + {"mac": "BB:BB:BB:BB:BB:03", "present": 1, + "parent_mac": "AA:AA:AA:AA:AA:03", "rel_type": "nic", "req_all": 0}, + ]) + device_handling.update_devPresentLastScan_based_on_nics(db) + assert _present(db, "AA:AA:AA:AA:AA:03") == 1 + + def test_req_all_mode_all_nics_online_raises_absent_parent(self): + db = _make_db([ + {"mac": "AA:AA:AA:AA:AA:04", "present": 0, + "parent_mac": "", "rel_type": "", "req_all": 1}, + {"mac": "BB:BB:BB:BB:BB:04a", "present": 1, + "parent_mac": "AA:AA:AA:AA:AA:04", "rel_type": "nic", "req_all": 0}, + {"mac": "BB:BB:BB:BB:BB:04b", "present": 1, + "parent_mac": "AA:AA:AA:AA:AA:04", "rel_type": "nic", "req_all": 0}, + ]) + device_handling.update_devPresentLastScan_based_on_nics(db) + assert _present(db, "AA:AA:AA:AA:AA:04") == 1 + + def test_req_all_mode_partial_nics_does_not_raise_absent_parent(self): + """In req_all mode, if not all NICs are online, an absent parent stays absent.""" + db = _make_db([ + {"mac": "AA:AA:AA:AA:AA:05", "present": 0, + "parent_mac": "", "rel_type": "", "req_all": 1}, + {"mac": "BB:BB:BB:BB:BB:05a", "present": 1, + "parent_mac": "AA:AA:AA:AA:AA:05", "rel_type": "nic", "req_all": 0}, + {"mac": "BB:BB:BB:BB:BB:05b", "present": 0, + "parent_mac": "AA:AA:AA:AA:AA:05", "rel_type": "nic", "req_all": 0}, + ]) + device_handling.update_devPresentLastScan_based_on_nics(db) + assert _present(db, "AA:AA:AA:AA:AA:05") == 0 + + def test_any_mode_all_nics_absent_leaves_parent_absent(self): + db = _make_db([ + {"mac": "AA:AA:AA:AA:AA:06", "present": 0, + "parent_mac": "", "rel_type": "", "req_all": 0}, + {"mac": "BB:BB:BB:BB:BB:06", "present": 0, + "parent_mac": "AA:AA:AA:AA:AA:06", "rel_type": "nic", "req_all": 0}, + ]) + device_handling.update_devPresentLastScan_based_on_nics(db) + assert _present(db, "AA:AA:AA:AA:AA:06") == 0 + + +# --------------------------------------------------------------------------- +# No NIC children → no change regardless of presence +# --------------------------------------------------------------------------- + +class TestNoNicChildren: + def test_parent_with_no_nics_unchanged(self): + db = _make_db([ + {"mac": "AA:AA:AA:AA:AA:07", "present": 1, + "parent_mac": "", "rel_type": "", "req_all": 0}, + {"mac": "AA:AA:AA:AA:AA:08", "present": 0, + "parent_mac": "", "rel_type": "", "req_all": 0}, + ]) + updated = device_handling.update_devPresentLastScan_based_on_nics(db) + assert updated == 0 + assert _present(db, "AA:AA:AA:AA:AA:07") == 1 + assert _present(db, "AA:AA:AA:AA:AA:08") == 0 From f52cc50705d01a9dc15ce529bc1290e40ade6437 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 08:52:43 +0000 Subject: [PATCH 3/5] fix: address review feedback on NIC presence logic and tests - Replace max() with explicit if/else for readability - Use db_test_helpers (make_db, make_device_dict, insert_device_from_dict, DummyDB) instead of local mock DB objects in test_nic_presence.py - Lowercase all MAC addresses in tests Co-authored-by: jokob-sk <96159884+jokob-sk@users.noreply.github.com> --- server/scan/device_handling.py | 15 ++- test/scan/test_nic_presence.py | 169 +++++++++++++++------------------ 2 files changed, 84 insertions(+), 100 deletions(-) diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 53dc6152f..1d4fa8993 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -1270,12 +1270,17 @@ def update_devPresentLastScan_based_on_nics(db): if nics: nic_statuses = [nic.get("devPresentLastScan") == 1 for nic in nics] if req_all: - nic_derived = int(all(nic_statuses)) + nic_online = all(nic_statuses) else: - nic_derived = int(any(nic_statuses)) - # NIC children can only raise a parent's presence, never lower it - # when the parent itself was directly detected as present this scan. - new_present = max(original, nic_derived) + nic_online = any(nic_statuses) + + if original == 1: + # Parent was directly detected this scan — NIC children cannot + # force it offline. Leave new_present = original (no change). + pass + else: + # Parent was not directly detected — NICs determine presence. + new_present = 1 if nic_online else 0 # Only add update if changed if original != new_present: diff --git a/test/scan/test_nic_presence.py b/test/scan/test_nic_presence.py index be82e59e3..abdd04a66 100644 --- a/test/scan/test_nic_presence.py +++ b/test/scan/test_nic_presence.py @@ -5,9 +5,12 @@ child's absence, producing an endless one-directional Connected event stream. """ -import sqlite3 +import sys +import os -import pytest +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, DummyDB from server.scan import device_handling @@ -16,51 +19,19 @@ # Helpers # --------------------------------------------------------------------------- -def _make_db(rows): - """Return a DummyDB-compatible object populated with the given Devices rows.""" - conn = sqlite3.connect(":memory:") - conn.row_factory = sqlite3.Row - cur = conn.cursor() - cur.execute( - """ - CREATE TABLE Devices ( - devMac TEXT PRIMARY KEY, - devPresentLastScan INTEGER DEFAULT 0, - devParentMAC TEXT, - devParentRelType TEXT DEFAULT '', - devReqNicsOnline INTEGER DEFAULT 0 - ) - """ - ) - cur.executemany( - """ - INSERT INTO Devices (devMac, devPresentLastScan, devParentMAC, - devParentRelType, devReqNicsOnline) - VALUES (:mac, :present, :parent_mac, :rel_type, :req_all) - """, - rows, - ) - conn.commit() - - class DummyDB: - def __init__(self, connection): - self.sql = connection.cursor() - self._conn = connection - - def commitDB(self): - self._conn.commit() - - db = DummyDB(conn) - # Re-use conn cursor for later reads - db._raw_conn = conn - return db - - -def _present(db, mac): - row = db._raw_conn.execute( +def _setup(devices: list[dict]): + """Return a DummyDB seeded with the given device dicts.""" + conn = make_db() + for dev in devices: + insert_device_from_dict(conn, dev) + return DummyDB(conn) + + +def _present(db: DummyDB, mac: str) -> int: + row = db._conn.execute( "SELECT devPresentLastScan FROM Devices WHERE devMac = ?", (mac,) ).fetchone() - return row[0] + return row["devPresentLastScan"] # --------------------------------------------------------------------------- @@ -68,32 +39,34 @@ def _present(db, mac): # --------------------------------------------------------------------------- class TestNicChildDoesNotForcePresentParentDown: - """Parent was directly detected this scan; absent NIC must NOT override that.""" + """Parent was directly detected this scan; an absent NIC must not override that.""" def test_any_mode_absent_nic_does_not_clear_present_parent(self): """Bug: req_all=0, parent present=1, nic present=0 → parent must stay 1.""" - db = _make_db([ - {"mac": "AA:AA:AA:AA:AA:01", "present": 1, - "parent_mac": "", "rel_type": "", "req_all": 0}, - {"mac": "BB:BB:BB:BB:BB:01", "present": 0, - "parent_mac": "AA:AA:AA:AA:AA:01", "rel_type": "nic", "req_all": 0}, + db = _setup([ + make_device_dict("aa:aa:aa:aa:aa:01", devPresentLastScan=1, + devParentMAC="", devParentRelType="", devReqNicsOnline=0), + make_device_dict("bb:bb:bb:bb:bb:01", devPresentLastScan=0, + devParentMAC="aa:aa:aa:aa:aa:01", + devParentRelType="nic", devReqNicsOnline=0), ]) device_handling.update_devPresentLastScan_based_on_nics(db) - assert _present(db, "AA:AA:AA:AA:AA:01") == 1, ( + assert _present(db, "aa:aa:aa:aa:aa:01") == 1, ( "Parent directly detected as present must not be forced offline " "by an absent NIC child." ) def test_req_all_mode_absent_nic_does_not_clear_present_parent(self): """Bug: req_all=1, parent present=1, nic present=0 → parent must stay 1.""" - db = _make_db([ - {"mac": "AA:AA:AA:AA:AA:02", "present": 1, - "parent_mac": "", "rel_type": "", "req_all": 1}, - {"mac": "BB:BB:BB:BB:BB:02", "present": 0, - "parent_mac": "AA:AA:AA:AA:AA:02", "rel_type": "nic", "req_all": 0}, + db = _setup([ + make_device_dict("aa:aa:aa:aa:aa:02", devPresentLastScan=1, + devParentMAC="", devParentRelType="", devReqNicsOnline=1), + make_device_dict("bb:bb:bb:bb:bb:02", devPresentLastScan=0, + devParentMAC="aa:aa:aa:aa:aa:02", + devParentRelType="nic", devReqNicsOnline=0), ]) device_handling.update_devPresentLastScan_based_on_nics(db) - assert _present(db, "AA:AA:AA:AA:AA:02") == 1 + assert _present(db, "aa:aa:aa:aa:aa:02") == 1 # --------------------------------------------------------------------------- @@ -101,52 +74,58 @@ def test_req_all_mode_absent_nic_does_not_clear_present_parent(self): # --------------------------------------------------------------------------- class TestNicRaisesAbsentParent: - """NIC children should be able to mark a parent present when it wasn't seen directly.""" + """NIC children should be able to mark a parent present when it was not seen directly.""" def test_any_mode_online_nic_raises_absent_parent(self): - db = _make_db([ - {"mac": "AA:AA:AA:AA:AA:03", "present": 0, - "parent_mac": "", "rel_type": "", "req_all": 0}, - {"mac": "BB:BB:BB:BB:BB:03", "present": 1, - "parent_mac": "AA:AA:AA:AA:AA:03", "rel_type": "nic", "req_all": 0}, + db = _setup([ + make_device_dict("aa:aa:aa:aa:aa:03", devPresentLastScan=0, + devParentMAC="", devParentRelType="", devReqNicsOnline=0), + make_device_dict("bb:bb:bb:bb:bb:03", devPresentLastScan=1, + devParentMAC="aa:aa:aa:aa:aa:03", + devParentRelType="nic", devReqNicsOnline=0), ]) device_handling.update_devPresentLastScan_based_on_nics(db) - assert _present(db, "AA:AA:AA:AA:AA:03") == 1 + assert _present(db, "aa:aa:aa:aa:aa:03") == 1 def test_req_all_mode_all_nics_online_raises_absent_parent(self): - db = _make_db([ - {"mac": "AA:AA:AA:AA:AA:04", "present": 0, - "parent_mac": "", "rel_type": "", "req_all": 1}, - {"mac": "BB:BB:BB:BB:BB:04a", "present": 1, - "parent_mac": "AA:AA:AA:AA:AA:04", "rel_type": "nic", "req_all": 0}, - {"mac": "BB:BB:BB:BB:BB:04b", "present": 1, - "parent_mac": "AA:AA:AA:AA:AA:04", "rel_type": "nic", "req_all": 0}, + db = _setup([ + make_device_dict("aa:aa:aa:aa:aa:04", devPresentLastScan=0, + devParentMAC="", devParentRelType="", devReqNicsOnline=1), + make_device_dict("bb:bb:bb:bb:bb:04", devPresentLastScan=1, + devParentMAC="aa:aa:aa:aa:aa:04", + devParentRelType="nic", devReqNicsOnline=0), + make_device_dict("cc:cc:cc:cc:cc:04", devPresentLastScan=1, + devParentMAC="aa:aa:aa:aa:aa:04", + devParentRelType="nic", devReqNicsOnline=0), ]) device_handling.update_devPresentLastScan_based_on_nics(db) - assert _present(db, "AA:AA:AA:AA:AA:04") == 1 + assert _present(db, "aa:aa:aa:aa:aa:04") == 1 def test_req_all_mode_partial_nics_does_not_raise_absent_parent(self): - """In req_all mode, if not all NICs are online, an absent parent stays absent.""" - db = _make_db([ - {"mac": "AA:AA:AA:AA:AA:05", "present": 0, - "parent_mac": "", "rel_type": "", "req_all": 1}, - {"mac": "BB:BB:BB:BB:BB:05a", "present": 1, - "parent_mac": "AA:AA:AA:AA:AA:05", "rel_type": "nic", "req_all": 0}, - {"mac": "BB:BB:BB:BB:BB:05b", "present": 0, - "parent_mac": "AA:AA:AA:AA:AA:05", "rel_type": "nic", "req_all": 0}, + """req_all=1: if not all NICs are online, an absent parent stays absent.""" + db = _setup([ + make_device_dict("aa:aa:aa:aa:aa:05", devPresentLastScan=0, + devParentMAC="", devParentRelType="", devReqNicsOnline=1), + make_device_dict("bb:bb:bb:bb:bb:05", devPresentLastScan=1, + devParentMAC="aa:aa:aa:aa:aa:05", + devParentRelType="nic", devReqNicsOnline=0), + make_device_dict("cc:cc:cc:cc:cc:05", devPresentLastScan=0, + devParentMAC="aa:aa:aa:aa:aa:05", + devParentRelType="nic", devReqNicsOnline=0), ]) device_handling.update_devPresentLastScan_based_on_nics(db) - assert _present(db, "AA:AA:AA:AA:AA:05") == 0 + assert _present(db, "aa:aa:aa:aa:aa:05") == 0 def test_any_mode_all_nics_absent_leaves_parent_absent(self): - db = _make_db([ - {"mac": "AA:AA:AA:AA:AA:06", "present": 0, - "parent_mac": "", "rel_type": "", "req_all": 0}, - {"mac": "BB:BB:BB:BB:BB:06", "present": 0, - "parent_mac": "AA:AA:AA:AA:AA:06", "rel_type": "nic", "req_all": 0}, + db = _setup([ + make_device_dict("aa:aa:aa:aa:aa:06", devPresentLastScan=0, + devParentMAC="", devParentRelType="", devReqNicsOnline=0), + make_device_dict("bb:bb:bb:bb:bb:06", devPresentLastScan=0, + devParentMAC="aa:aa:aa:aa:aa:06", + devParentRelType="nic", devReqNicsOnline=0), ]) device_handling.update_devPresentLastScan_based_on_nics(db) - assert _present(db, "AA:AA:AA:AA:AA:06") == 0 + assert _present(db, "aa:aa:aa:aa:aa:06") == 0 # --------------------------------------------------------------------------- @@ -155,13 +134,13 @@ def test_any_mode_all_nics_absent_leaves_parent_absent(self): class TestNoNicChildren: def test_parent_with_no_nics_unchanged(self): - db = _make_db([ - {"mac": "AA:AA:AA:AA:AA:07", "present": 1, - "parent_mac": "", "rel_type": "", "req_all": 0}, - {"mac": "AA:AA:AA:AA:AA:08", "present": 0, - "parent_mac": "", "rel_type": "", "req_all": 0}, + db = _setup([ + make_device_dict("aa:aa:aa:aa:aa:07", devPresentLastScan=1, + devParentMAC="", devParentRelType="", devReqNicsOnline=0), + make_device_dict("aa:aa:aa:aa:aa:08", devPresentLastScan=0, + devParentMAC="", devParentRelType="", devReqNicsOnline=0), ]) updated = device_handling.update_devPresentLastScan_based_on_nics(db) assert updated == 0 - assert _present(db, "AA:AA:AA:AA:AA:07") == 1 - assert _present(db, "AA:AA:AA:AA:AA:08") == 0 + assert _present(db, "aa:aa:aa:aa:aa:07") == 1 + assert _present(db, "aa:aa:aa:aa:aa:08") == 0 From 66db9a4b57d334cc608fbc6dfa07ddb534dd2cae Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:04:36 +0000 Subject: [PATCH 4/5] chore: add missing pr-analysis and logging-standards skills Co-authored-by: jokob-sk <96159884+jokob-sk@users.noreply.github.com> --- .gemini/skills/logging-standards/SKILL.md | 67 +++++++++++++++++++++++ .gemini/skills/pr-analysis/SKILL.md | 52 ++++++++++++++++++ .gemini/skills/skills-index/SKILL.md | 2 + .github/skills/logging-standards/SKILL.md | 67 +++++++++++++++++++++++ .github/skills/pr-analysis/SKILL.md | 53 ++++++++++++++++++ .github/skills/skills-overview/SKILL.md | 2 + 6 files changed, 243 insertions(+) create mode 100644 .gemini/skills/logging-standards/SKILL.md create mode 100644 .gemini/skills/pr-analysis/SKILL.md create mode 100644 .github/skills/logging-standards/SKILL.md create mode 100644 .github/skills/pr-analysis/SKILL.md diff --git a/.gemini/skills/logging-standards/SKILL.md b/.gemini/skills/logging-standards/SKILL.md new file mode 100644 index 000000000..8395586b0 --- /dev/null +++ b/.gemini/skills/logging-standards/SKILL.md @@ -0,0 +1,67 @@ +--- +name: logging-standards +description: Logging conventions for NetAlertX backend Python code. Use this when adding, modifying, or reviewing log statements. +--- + +# Logging Standards + +## Import + +```python +from logger import mylog +``` + +Never import `logging` directly in application code. Use `mylog` exclusively. + +## Function Signature + +```python +mylog(level, message_or_list) +``` + +`message_or_list` can be a plain string or a list of values — the logger joins them with spaces. + +## Log Levels + +Levels from least to most verbose (higher number = more output): + +| Level | Numeric | When to use | +|-------|---------|-------------| +| `"none"` | 0 | Always printed regardless of user setting. Reserve for startup, fatal errors, and one-time permission checks. | +| `"minimal"` | 1 | Important state transitions visible by default (scan start/end, plugin finish, restart). | +| `"verbose"` | 2 | Informational progress — what the system is doing without clutter (e.g. "No changes to report"). | +| `"debug"` | 3 | Developer-level detail — loop decisions, branch taken, counts. | +| `"trace"` | 4 | Granular per-item tracing — individual device rows, SQL queries, raw values. | + +## Message Format + +Prefix every message with a `[Module]` tag matching the file/function context: + +```python +mylog("debug", [f"[device_handling] Processing MAC: {mac}"]) +mylog("verbose", ["[Scan] Scan complete — devices updated:", count]) +``` + +Use `f-strings` inside a list element, not string concatenation: + +```python +# Correct +mylog("debug", [f"[NIC] parent={parent_mac} nic_online={nic_online}"]) + +# Avoid +mylog("debug", "[NIC] parent=" + parent_mac + " nic_online=" + str(nic_online)) +``` + +## Timestamp + +`mylog` / `file_print` prepend the current local-timezone time automatically via `timeNowTZ`. Do **not** add a timestamp manually inside the message. + +## What NOT to Log + +- Do not log raw user input without sanitization. +- Do not log full SQL query strings at `"none"` or `"minimal"` — use `"trace"` at most. +- Do not use `print()` in server code — use `mylog`. `file_print` is an internal helper; do not call it directly. + +## Log File Location + +Written to `{logPath}/app.log` (`logPath` from `const.py` → `/tmp/logs` at runtime). Do not hardcode this path. diff --git a/.gemini/skills/pr-analysis/SKILL.md b/.gemini/skills/pr-analysis/SKILL.md new file mode 100644 index 000000000..26dd7793a --- /dev/null +++ b/.gemini/skills/pr-analysis/SKILL.md @@ -0,0 +1,52 @@ +--- +name: pr-analysis +description: How to analyze and respond to GitHub PR review comments in NetAlertX. Use this whenever you are addressing PR feedback, review threads, or inline code comments. +--- + +# PR Analysis + +## Before Acting on Any PR Comment + +1. Load `code-standards` skill — all code changes must comply with it before replying. +2. Load `testing-workflow` skill — any test additions or changes must follow it. +3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings` for config). + +## Comment Classification + +For each comment, determine: + +| Type | Action | +|------|--------| +| Request for code change | Make the change, validate it, then reply with the short commit hash | +| Question about code | Reply with a concise answer (no restatement of the question) | +| Suggestion / feedback | Decide if it is actionable. If yes, act and reply. If not, do not reply. | +| General / praise | Do not reply. | + +## Acting on Comments — Step by Step + +1. **Identify all actionable comments** before touching any file. +2. **Load relevant skills** to understand conventions that apply. +3. **Prepare a plan** — list each file and the exact change required. +4. **Make changes one comment at a time** — keep commits focused. +5. **Run targeted tests** after each change (`testing-workflow` skill). +6. **Reply** only after the commit is pushed. Include the short SHA. + +## Reply Guidelines + +- Be concise. Do not summarize or restate the original comment. +- State what was done and (optionally) why. +- Include the short commit hash when relevant. +- Do not thank or compliment the reviewer. + +## What to Check After Every Batch of Changes + +- All MACs are lowercase everywhere (code-standards). +- No mocks or DB helpers are re-defined locally — use `test/db_test_helpers.py` (code-standards). +- No inline imports — all imports at the top of the file (code-standards). +- Tests live under a subdirectory of `test/` matching the source path, not in `test/` root (code-standards). + +## Stacked / Base-Branch Issues + +When a PR targets a non-default branch (e.g. `next_release`): +- Do **not** retarget the branch yourself; note it in a reply so the author can do it from the GitHub UI. +- Check CI failures on the **base branch** first before checking your branch. diff --git a/.gemini/skills/skills-index/SKILL.md b/.gemini/skills/skills-index/SKILL.md index d5272a5d9..ea21fd9c0 100644 --- a/.gemini/skills/skills-index/SKILL.md +++ b/.gemini/skills/skills-index/SKILL.md @@ -24,6 +24,8 @@ Skills with the same purpose exist in both, sometimes under different names and | Project navigation | `project-navigation` | `project-navigation` | Copilot version has full path tables and env vars; Gemini version is a brief reference | | Plugin dev | `plugin-development` | `plugin-run-development` | Copilot version is comprehensive (data contract, phases, formats); Gemini version is a brief checklist pointing to `docs/PLUGINS_DEV.md` | | Devcontainer | `devcontainer-management` | `devcontainer-services` + `devcontainer-setup` + `devcontainer-configs` | Gemini combines into one (uses `docker exec`); Copilot splits into 3 focused skills | +| PR review | `pr-analysis` | `pr-analysis` | How to classify and respond to PR comments; pre-flight skill loading checklist | +| Logging | `logging-standards` | `logging-standards` | `mylog` levels, message format, what not to log | --- diff --git a/.github/skills/logging-standards/SKILL.md b/.github/skills/logging-standards/SKILL.md new file mode 100644 index 000000000..8cbc834ca --- /dev/null +++ b/.github/skills/logging-standards/SKILL.md @@ -0,0 +1,67 @@ +--- +name: netalertx-logging-standards +description: Logging conventions for NetAlertX backend Python code. Use this when adding, modifying, or reviewing log statements. +--- + +# Logging Standards + +## Import + +```python +from logger import mylog +``` + +Never import `logging` directly in application code. Use `mylog` exclusively. + +## Function Signature + +```python +mylog(level, message_or_list) +``` + +`message_or_list` can be a plain string or a list of values — the logger joins them with spaces. + +## Log Levels + +Levels from least to most verbose (higher number = more output): + +| Level | Numeric | When to use | +|-------|---------|-------------| +| `"none"` | 0 | Always printed regardless of user setting. Reserve for startup, fatal errors, and one-time permission checks. | +| `"minimal"` | 1 | Important state transitions visible by default (scan start/end, plugin finish, restart). | +| `"verbose"` | 2 | Informational progress — what the system is doing without clutter (e.g. "No changes to report"). | +| `"debug"` | 3 | Developer-level detail — loop decisions, branch taken, counts. | +| `"trace"` | 4 | Granular per-item tracing — individual device rows, SQL queries, raw values. | + +## Message Format + +Prefix every message with a `[Module]` tag matching the file/function context: + +```python +mylog("debug", [f"[device_handling] Processing MAC: {mac}"]) +mylog("verbose", ["[Scan] Scan complete — devices updated:", count]) +``` + +Use `f-strings` inside a list element, not string concatenation: + +```python +# Correct +mylog("debug", [f"[NIC] parent={parent_mac} nic_online={nic_online}"]) + +# Avoid +mylog("debug", "[NIC] parent=" + parent_mac + " nic_online=" + str(nic_online)) +``` + +## Timestamp + +`mylog` / `file_print` prepend the current local-timezone time automatically via `timeNowTZ`. Do **not** add a timestamp manually inside the message. + +## What NOT to Log + +- Do not log raw user input without sanitization. +- Do not log full SQL query strings at `"none"` or `"minimal"` — use `"trace"` at most. +- Do not use `print()` in server code — use `mylog`. `file_print` is an internal helper; do not call it directly. + +## Log File Location + +Written to `{logPath}/app.log` (`logPath` from `const.py` → `/tmp/logs` at runtime). Do not hardcode this path. diff --git a/.github/skills/pr-analysis/SKILL.md b/.github/skills/pr-analysis/SKILL.md new file mode 100644 index 000000000..bea79ca55 --- /dev/null +++ b/.github/skills/pr-analysis/SKILL.md @@ -0,0 +1,53 @@ +--- +name: netalertx-pr-analysis +description: How to analyze and respond to GitHub PR review comments in NetAlertX. Use this whenever you are addressing PR feedback, review threads, or inline code comments. +--- + +# PR Analysis + +## Before Acting on Any PR Comment + +1. Load `code-standards` skill — all code changes must comply with it before replying. +2. Load `testing-workflow` skill — any test additions or changes must follow it. +3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings-management` for config). + +## Comment Classification + +For each comment, determine: + +| Type | Action | +|------|--------| +| Request for code change | Make the change, validate it, then reply with the short commit hash | +| Question about code | Reply with a concise answer (no restatement of the question) | +| Suggestion / feedback | Decide if it is actionable. If yes, act and reply. If not, do not reply. | +| General / praise | Do not reply. | + +## Acting on Comments — Step by Step + +1. **Identify all actionable comments** before touching any file. +2. **Load relevant skills** to understand conventions that apply. +3. **Prepare a plan** — list each file and the exact change required. +4. **Make changes one comment at a time** — keep commits focused. +5. **Run targeted tests** after each change (`testing-workflow` skill). +6. **Reply** only after the commit is pushed via `report_progress`. Include the short SHA. + +## Reply Guidelines + +- Be concise. Do not summarize or restate the original comment. +- State what was done and (optionally) why. +- Include the short commit hash when relevant. +- Do not thank or compliment the reviewer. + +## What to Check After Every Batch of Changes + +- All MACs are lowercase everywhere (code-standards). +- No mocks or DB helpers are re-defined locally — use `test/db_test_helpers.py` (code-standards). +- No inline imports — all imports at the top of the file (code-standards). +- Tests live under a subdirectory of `test/` matching the source path, not in `test/` root (code-standards). +- Secret scan (`runtime-tools-secret_scanning`) before committing. + +## Stacked / Base-Branch Issues + +When a PR targets a non-default branch (e.g. `next_release`): +- Do **not** retarget the branch yourself; note it in a reply so the author can do it from the GitHub UI. +- Check CI failures on the **base branch** first before checking your branch. diff --git a/.github/skills/skills-overview/SKILL.md b/.github/skills/skills-overview/SKILL.md index f526459ec..e02b5f96f 100644 --- a/.github/skills/skills-overview/SKILL.md +++ b/.github/skills/skills-overview/SKILL.md @@ -24,6 +24,8 @@ Skills with the same purpose exist in both, sometimes under different names and | Project navigation | `project-navigation` | `project-navigation` | Copilot version has full path tables and env vars; Gemini version is a brief reference | | Plugin dev | `plugin-run-development` | `plugin-development` | Copilot version is comprehensive (data contract, phases, formats); Gemini version is a brief checklist pointing to `docs/PLUGINS_DEV.md` | | Devcontainer | `devcontainer-services` + `devcontainer-setup` + `devcontainer-configs` | `devcontainer-management` | Copilot splits into 3 focused skills; Gemini combines into one (uses `docker exec`) | +| PR review | `pr-analysis` | `pr-analysis` | How to classify and respond to PR comments; pre-flight skill loading checklist | +| Logging | `logging-standards` | `logging-standards` | `mylog` levels, message format, what not to log | --- From 4ed96e9304126249b2656461278b6d793a114c7b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:14:50 +0000 Subject: [PATCH 5/5] chore: strengthen test MAC and helper rules in code-standards and pr-analysis skills Co-authored-by: jokob-sk <96159884+jokob-sk@users.noreply.github.com> --- .gemini/skills/pr-analysis/SKILL.md | 17 +++++++++++++---- .github/skills/code-standards/SKILL.md | 15 +++++++++++++++ .github/skills/pr-analysis/SKILL.md | 17 +++++++++++++---- 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/.gemini/skills/pr-analysis/SKILL.md b/.gemini/skills/pr-analysis/SKILL.md index 26dd7793a..be9b408eb 100644 --- a/.gemini/skills/pr-analysis/SKILL.md +++ b/.gemini/skills/pr-analysis/SKILL.md @@ -5,6 +5,15 @@ description: How to analyze and respond to GitHub PR review comments in NetAlert # PR Analysis +## Before Writing Any Test Code — Non-Negotiable Checklist + +Run through this before creating or editing any file under `test/`: + +1. **Helpers first:** Check `test/db_test_helpers.py` for existing factories (`make_db`, `make_device_dict`, `insert_device_from_dict`, `DummyDB`). Use them. If what you need doesn't exist, add it there — never define it locally in the test file. +2. **MAC literals must be lowercase:** Every MAC string in fixtures, parametrize, assertions, docstrings, and comments must be lowercase hex (e.g. `aa:bb:cc:dd:ee:01`). No exceptions. +3. **Test file location:** Place tests under a subdirectory of `test/` that mirrors the source path (e.g. `test/scan/` for `server/scan/`). Never put test files directly in `test/`. +4. **No inline imports:** All imports at the top of the file. + ## Before Acting on Any PR Comment 1. Load `code-standards` skill — all code changes must comply with it before replying. @@ -40,10 +49,10 @@ For each comment, determine: ## What to Check After Every Batch of Changes -- All MACs are lowercase everywhere (code-standards). -- No mocks or DB helpers are re-defined locally — use `test/db_test_helpers.py` (code-standards). -- No inline imports — all imports at the top of the file (code-standards). -- Tests live under a subdirectory of `test/` matching the source path, not in `test/` root (code-standards). +- **MAC literals lowercase** — grep for uppercase hex in every changed test file: `grep -Pn '[0-9A-F]{2}:[0-9A-F]' test/` must be empty. +- **No local DB helpers** — no `DummyDB`, `make_db`, or inline DDL defined outside `test/db_test_helpers.py`. +- No inline imports — all imports at the top of the file. +- Tests live under a subdirectory of `test/` matching the source path, not in `test/` root. ## Stacked / Base-Branch Issues diff --git a/.github/skills/code-standards/SKILL.md b/.github/skills/code-standards/SKILL.md index 83c52d0a8..e009feefd 100644 --- a/.github/skills/code-standards/SKILL.md +++ b/.github/skills/code-standards/SKILL.md @@ -97,6 +97,21 @@ from db_test_helpers import make_db, DummyDB, insert_device, minutes_ago If a helper you need doesn't exist yet, add it to `db_test_helpers.py` — not locally in the test file. +## MAC Literals in Tests — ALWAYS Lowercase + +**MANDATORY:** Every MAC address literal used in test fixtures, parametrize decorators, assertions, or comments must be lowercase hex: + +```python +# Correct +make_device_dict("aa:bb:cc:dd:ee:01", ...) + +# Wrong — will be rejected in review +make_device_dict("AA:BB:CC:DD:EE:01", ...) +make_device_dict("Aa:Bb:Cc:Dd:Ee:01", ...) +``` + +This applies to hardcoded strings in `assert`, `pytest.mark.parametrize`, docstrings, and comments too. There are no exceptions. + ## Path Hygiene - Use environment variables for runtime paths diff --git a/.github/skills/pr-analysis/SKILL.md b/.github/skills/pr-analysis/SKILL.md index bea79ca55..904d9f12a 100644 --- a/.github/skills/pr-analysis/SKILL.md +++ b/.github/skills/pr-analysis/SKILL.md @@ -5,6 +5,15 @@ description: How to analyze and respond to GitHub PR review comments in NetAlert # PR Analysis +## Before Writing Any Test Code — Non-Negotiable Checklist + +Run through this before creating or editing any file under `test/`: + +1. **Helpers first:** Check `test/db_test_helpers.py` for existing factories (`make_db`, `make_device_dict`, `insert_device_from_dict`, `DummyDB`). Use them. If what you need doesn't exist, add it there — never define it locally in the test file. +2. **MAC literals must be lowercase:** Every MAC string in fixtures, `parametrize`, assertions, docstrings, and comments must be lowercase hex (e.g. `aa:bb:cc:dd:ee:01`). No exceptions. +3. **Test file location:** Place tests under a subdirectory of `test/` that mirrors the source path (e.g. `test/scan/` for `server/scan/`). Never put test files directly in `test/`. +4. **No inline imports:** All imports at the top of the file. + ## Before Acting on Any PR Comment 1. Load `code-standards` skill — all code changes must comply with it before replying. @@ -40,10 +49,10 @@ For each comment, determine: ## What to Check After Every Batch of Changes -- All MACs are lowercase everywhere (code-standards). -- No mocks or DB helpers are re-defined locally — use `test/db_test_helpers.py` (code-standards). -- No inline imports — all imports at the top of the file (code-standards). -- Tests live under a subdirectory of `test/` matching the source path, not in `test/` root (code-standards). +- **MAC literals lowercase** — grep for uppercase hex in every changed test file: `grep -Pn '[0-9A-F]{2}:[0-9A-F]' test/` must be empty. +- **No local DB helpers** — no `DummyDB`, `make_db`, or inline DDL defined outside `test/db_test_helpers.py`. +- No inline imports — all imports at the top of the file. +- Tests live under a subdirectory of `test/` matching the source path, not in `test/` root. - Secret scan (`runtime-tools-secret_scanning`) before committing. ## Stacked / Base-Branch Issues