From 6dbd3f8f290f84928a976ece48576c6da75bf195 Mon Sep 17 00:00:00 2001 From: Panagiotis Koilakos <45639356+Svestis@users.noreply.github.com> Date: Mon, 28 Sep 2026 01:38:35 +0200 Subject: [PATCH 01/13] BE: escape backslashes when serializing string settings to app.conf app.conf is written by util.php saveSettings() as Python source and later compiled/exec'd by the backend. String settings were emitted into single-quoted Python literals with only ' encoded (as {s-quote}), so backslashes were written raw. A regex such as 192\.0\.2\..* then produced "SyntaxWarning: invalid escape sequence '\.'", valid Python escapes such as \n were silently reinterpreted, and a trailing backslash made the file fail to compile. Move the encoder into a side-effect-free helper, app_conf_encode.php, as encode_python_string(). It now doubles backslashes before the existing {s-quote} replacement, so backslashes round-trip unchanged through app.conf serialization and Python parsing, while single quotes keep using the legacy {s-quote} placeholder. Both scalar string and array string serialization use the helper. Add regression tests that run the real PHP helper through the PHP CLI, compile the generated source with warnings promoted to errors, and check the scalar and array round trips. --- front/php/server/app_conf_encode.php | 18 ++++ front/php/server/util.php | 10 +- test/backend/test_app_conf_string_escaping.py | 96 +++++++++++++++++++ 3 files changed, 117 insertions(+), 7 deletions(-) create mode 100644 front/php/server/app_conf_encode.php create mode 100644 test/backend/test_app_conf_string_escaping.py diff --git a/front/php/server/app_conf_encode.php b/front/php/server/app_conf_encode.php new file mode 100644 index 000000000..1690c1ef2 --- /dev/null +++ b/front/php/server/app_conf_encode.php @@ -0,0 +1,18 @@ + 3 && is_array($settingValue) == true) { foreach ($settingValue as $val) { - $temp .= "'" . encode_single_quotes($val) . "',"; + $temp .= "'" . encode_python_string($val) . "',"; } $temp = substr_replace($temp, "", -1); // remove last comma ',' @@ -271,11 +272,6 @@ function getSettingValue($setKey) { return 'Could not find setting '.$setKey; } -// ------------------------------------------------------------------------------------------- -function encode_single_quotes ($val) { - $result = str_replace ('\'','{s-quote}',$val); - return $result; -} // ------------------------------------------------------------------------------------------- // Helper function to send notifications via the backend API endpoint // ------------------------------------------------------------------------------------------- diff --git a/test/backend/test_app_conf_string_escaping.py b/test/backend/test_app_conf_string_escaping.py new file mode 100644 index 000000000..cb5db3812 --- /dev/null +++ b/test/backend/test_app_conf_string_escaping.py @@ -0,0 +1,96 @@ +""" +NetAlertX app.conf String Escaping Tests + +Runs the real PHP encode_python_string() (front/php/server/app_conf_encode.php) +through the PHP CLI, embeds its output in app.conf-style Python source the same +way util.php saveSettings() does, and checks that the source compiles without +warnings and parses back to the typed value (with ' mapped to {s-quote}). + +License: GNU GPLv3 +""" + +import json +import shutil +import subprocess +import warnings +from pathlib import Path + +import pytest + +ENCODER_PHP = Path(__file__).resolve().parents[2] / "front" / "php" / "server" / "app_conf_encode.php" +PHP_BIN = shutil.which("php") or shutil.which("php83") + +pytestmark = pytest.mark.skipif(PHP_BIN is None, reason="PHP CLI (php or php83) not available") + +# Reads {"path": ..., "cases": [...]} from stdin and prints the encoded cases as JSON. +PHP_RUNNER = ( + '$in = json_decode(stream_get_contents(STDIN), true);' + 'require $in["path"];' + 'echo json_encode(array_map("encode_python_string", $in["cases"]));' +) + +CASES = { + "ordinary_text": "hello world", + "doc_regex": r"192\.0\.2\..*", + "consecutive_backslashes": r"a\\b", + "backslashes_only": "\\" * 3, + "trailing_backslash": "trail" + "\\", + "existing_s_quote": "x{s-quote}y", + "literal_single_quote": "it's", + "regex_with_quote": r"\d+\s*'", + "backslash_before_quote": r"a\'b", +} + + +@pytest.fixture(scope="module") +def encoded(): + """Return {case_id: encoded} produced by one PHP CLI run of encode_python_string().""" + payload = json.dumps({"path": str(ENCODER_PHP), "cases": list(CASES.values())}) + result = subprocess.run( + [PHP_BIN, "-r", PHP_RUNNER], + input=payload, + capture_output=True, + text=True, + timeout=60, + check=True, + ) + return dict(zip(CASES.keys(), json.loads(result.stdout))) + + +def parse_app_conf(source): + """Compile source with all warnings as errors and exec it like the backend app.conf readers.""" + with warnings.catch_warnings(): + warnings.simplefilter("error") + code = compile(source, "app.conf", "exec") + conf = {} + exec(code, {"__builtins__": {}}, conf) + return conf + + +def test_warning_check_rejects_unescaped_backslash(): + """The warnings-as-errors compile rejects the unescaped regex source that util.php emitted before escaping.""" + with pytest.raises(SyntaxError): + parse_app_conf(r"X='192\.0\.2\..*'") + + +@pytest.mark.parametrize("case_id", CASES.keys()) +def test_scalar_string_round_trip(encoded, case_id): + """A scalar string setting (X='') compiles cleanly and parses back to the typed value.""" + typed = CASES[case_id] + conf = parse_app_conf(f"X='{encoded[case_id]}'\n") + assert conf["X"] == typed.replace("'", "{s-quote}") + + +@pytest.mark.parametrize("case_id", CASES.keys()) +def test_array_string_round_trip(encoded, case_id): + """An array setting element (X=['plain','']) compiles cleanly and parses back to the typed value.""" + typed = CASES[case_id] + conf = parse_app_conf(f"X=['plain','{encoded[case_id]}']\n") + assert conf["X"] == ["plain", typed.replace("'", "{s-quote}")] + + +def test_doc_regex_exact_source(encoded): + """The documentation regex is emitted with doubled backslashes and parses back unchanged.""" + source = f"ICMP_IN_REGEX='{encoded['doc_regex']}'" + assert source == r"ICMP_IN_REGEX='192\\.0\\.2\\..*'" + assert parse_app_conf(source)["ICMP_IN_REGEX"] == r"192\.0\.2\..*" From 1c3139bb7bfd40a3b6b094f237c62488b51d6004 Mon Sep 17 00:00:00 2001 From: Panagiotis Koilakos <45639356+Svestis@users.noreply.github.com> Date: Tue, 29 Sep 2026 23:11:07 +0200 Subject: [PATCH 02/13] BE: address app.conf escaping review feedback Keep encode_python_string() in front/php/server/util.php, replacing encode_single_quotes() in place, and remove the standalone app_conf_encode.php helper file and its require. Rework the regression test to dispatch the real util.php savesettings path through the PHP CLI against temporary synthetic config, API and session directories, covering both scalar string and array string settings: the generated app.conf must compile with warnings as errors and parse back to the typed values. The existing {s-quote} lifecycle is preserved (single quotes are still written as {s-quote} and converted back by the same consumers), and the app.conf readers in initialise.py and plugin_helper.py are unchanged. --- front/php/server/app_conf_encode.php | 18 --- front/php/server/util.php | 10 +- test/backend/test_app_conf_string_escaping.py | 108 +++++++++++++----- 3 files changed, 88 insertions(+), 48 deletions(-) delete mode 100644 front/php/server/app_conf_encode.php diff --git a/front/php/server/app_conf_encode.php b/front/php/server/app_conf_encode.php deleted file mode 100644 index 1690c1ef2..000000000 --- a/front/php/server/app_conf_encode.php +++ /dev/null @@ -1,18 +0,0 @@ - "savesettings", "settings" => json_encode($in["settings"])];' + 'require $in["front"] . "/php/server/util.php";' ) +# Minimal app.conf that lets globals.php and security.php load without a password prompt. +SEED_APP_CONF = "TIMEZONE='UTC'\nSETPWD_enable_password=False\n" + CASES = { "ordinary_text": "hello world", "doc_regex": r"192\.0\.2\..*", @@ -42,19 +51,52 @@ CASES = { } +def scalar_key(case_id): + """Return the app.conf key used for the scalar string setting of a case.""" + return f"S_{case_id.upper()}" + + +def array_key(case_id): + """Return the app.conf key used for the array string setting of a case.""" + return f"A_{case_id.upper()}" + + @pytest.fixture(scope="module") -def encoded(): - """Return {case_id: encoded} produced by one PHP CLI run of encode_python_string().""" - payload = json.dumps({"path": str(ENCODER_PHP), "cases": list(CASES.values())}) +def app_conf(tmp_path_factory): + """Run util.php saveSettings() once for all cases and return the generated app.conf source.""" + root = tmp_path_factory.mktemp("app_conf") + config_dir = root / "config" + api_dir = root / "api" + session_dir = root / "session" + for folder in (config_dir, api_dir, session_dir): + folder.mkdir() + (config_dir / "app.conf").write_text(SEED_APP_CONF) + + # UI_WAIT_FOR_SETTINGS=True keeps getReloadWaitRequired() from reading the (absent) API files. + settings = [["General", "UI_WAIT_FOR_SETTINGS", "boolean", True]] + for case_id, typed in CASES.items(): + settings.append(["Test", scalar_key(case_id), "string", typed]) + settings.append(["Test", array_key(case_id), "array", ["plain", typed]]) + + env = dict(os.environ, NETALERTX_CONFIG=str(config_dir), NETALERTX_API=str(api_dir)) result = subprocess.run( - [PHP_BIN, "-r", PHP_RUNNER], - input=payload, + [PHP_BIN, "-d", f"session.save_path={session_dir}", "-d", "display_errors=stderr", "-r", PHP_RUNNER], + input=json.dumps({"front": str(FRONT_DIR), "settings": settings}), capture_output=True, text=True, + env=env, timeout=60, check=True, ) - return dict(zip(CASES.keys(), json.loads(result.stdout))) + assert json.loads(result.stdout)["success"] is True, result.stdout + result.stderr + return (config_dir / "app.conf").read_text() + + +def setting_line(source, key): + """Return the single app.conf line that assigns key.""" + lines = [line for line in source.splitlines() if line.startswith(f"{key}=")] + assert len(lines) == 1, f"expected one {key}= line, got {lines}" + return lines[0] def parse_app_conf(source): @@ -73,24 +115,32 @@ def test_warning_check_rejects_unescaped_backslash(): parse_app_conf(r"X='192\.0\.2\..*'") -@pytest.mark.parametrize("case_id", CASES.keys()) -def test_scalar_string_round_trip(encoded, case_id): - """A scalar string setting (X='') compiles cleanly and parses back to the typed value.""" - typed = CASES[case_id] - conf = parse_app_conf(f"X='{encoded[case_id]}'\n") - assert conf["X"] == typed.replace("'", "{s-quote}") +def test_generated_app_conf_compiles(app_conf): + """The whole app.conf written by saveSettings() compiles without warnings.""" + parse_app_conf(app_conf) @pytest.mark.parametrize("case_id", CASES.keys()) -def test_array_string_round_trip(encoded, case_id): - """An array setting element (X=['plain','']) compiles cleanly and parses back to the typed value.""" - typed = CASES[case_id] - conf = parse_app_conf(f"X=['plain','{encoded[case_id]}']\n") - assert conf["X"] == ["plain", typed.replace("'", "{s-quote}")] +def test_scalar_string_round_trip(app_conf, case_id): + """A scalar string setting written by saveSettings() compiles cleanly and parses back to the typed value.""" + key = scalar_key(case_id) + conf = parse_app_conf(setting_line(app_conf, key)) + assert conf[key] == CASES[case_id].replace("'", "{s-quote}") -def test_doc_regex_exact_source(encoded): +@pytest.mark.parametrize("case_id", CASES.keys()) +def test_array_string_round_trip(app_conf, case_id): + """An array string setting written by saveSettings() compiles cleanly and parses back to the typed values.""" + key = array_key(case_id) + conf = parse_app_conf(setting_line(app_conf, key)) + assert conf[key] == ["plain", CASES[case_id].replace("'", "{s-quote}")] + + +def test_doc_regex_exact_source(app_conf): """The documentation regex is emitted with doubled backslashes and parses back unchanged.""" - source = f"ICMP_IN_REGEX='{encoded['doc_regex']}'" - assert source == r"ICMP_IN_REGEX='192\\.0\\.2\\..*'" - assert parse_app_conf(source)["ICMP_IN_REGEX"] == r"192\.0\.2\..*" + scalar = setting_line(app_conf, scalar_key("doc_regex")) + array = setting_line(app_conf, array_key("doc_regex")) + assert scalar == r"S_DOC_REGEX='192\\.0\\.2\\..*'" + assert array == r"A_DOC_REGEX=['plain','192\\.0\\.2\\..*']" + assert parse_app_conf(scalar)["S_DOC_REGEX"] == r"192\.0\.2\..*" + assert parse_app_conf(array)["A_DOC_REGEX"] == ["plain", r"192\.0\.2\..*"] From 7ca46a5c0d0788f55b3da51ebfcb5f37cee040f0 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Thu, 1 Oct 2026 20:37:19 +1000 Subject: [PATCH 03/13] DOCS: skill cleanup --- .claude/skills/prd-writing/SKILL.md | 2 +- .gemini/skills/pr-analysis/SKILL.md | 4 ++-- .gemini/skills/prd-writing/SKILL.md | 2 +- .github/skills/pr-analysis/SKILL.md | 4 ++-- .github/skills/prd-writing/SKILL.md | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md index 374fbcd0e..1cca925d8 100644 --- a/.claude/skills/prd-writing/SKILL.md +++ b/.claude/skills/prd-writing/SKILL.md @@ -26,7 +26,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 4. **For every mechanism, trace every downstream consumer — not just the first one you find.** The single highest-value question before calling a design complete: "where else does this exact same check or logic get independently re-derived?" In a codebase without one source of truth for a concept (e.g. "is this record currently active" computed by three different queries in three different files), patching the first occurrence and stopping is the most common way a design ships with a hidden, silent gap. Grep for the pattern, not just the function you already know about. 5. **Record rejected alternatives with the reasoning, not just the chosen design.** Give it its own subsection (`### Rejected: X`). Without this, a future reader — or your own future self — re-proposes the rejected idea because the "why not" only ever existed in a conversation, not in the document. 6. **Force every open question to an explicit decision**, even if the decision is "accept as-is for v1, revisit if feedback says otherwise." An open question left unresolved in a PRD gets silently decided by whoever implements it — usually differently than anyone intended. -7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). +7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason (a real assertion mismatch, not a collection/import error) before writing a line of the fix. A test that never failed red can't be trusted to have caught anything - it's usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. 8. **Ask explicitly whether validating this needs real end-to-end infrastructure** (a new or modified plugin, a UI click-through) or whether synthetic unit-level fixtures suffice — don't assume either way. Check whether the functions under test take a DB connection/dict/list as a parameter (testable in isolation, no real plugin needed) or require a real file on disk (harder to fake, may need one). 9. **Ask explicitly whether this feature/mechanism should be exposed via the API, and give a recommendation, not just flag it as an open question.** This codebase already has three real API surfaces to consider extending rather than inventing a fourth: REST (`server/api_server/*_endpoint.py`), GraphQL (`graphql_endpoint.py`), and a Prometheus `/metrics` endpoint (`prometheus_endpoint.py`). Skipping this question doesn't mean "no API access needed"; it means the answer gets silently decided later by whoever first wants to query the new data externally, usually as its own separate feature request re-litigating a design this PRD already had full context to settle. Not everything needs exposure: purely internal/diagnostic state with no plausible external consumer doesn't, but say so explicitly, with the reason, rather than leaving it unaddressed. 10. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` for *every* index on the tables involved, not just the first one you find — a column can have both a plain index and a separate expression index (e.g. `idx_eve_mac_date_type ON Events(eveMac, ...)` alongside `idx_eve_lower_mac_date_type ON Events(LOWER(eveMac), ...)`), and missing the second one produces a wrong verdict. Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count, built against the *complete* real index set (copy every `CREATE INDEX` for the table, or run it against an actual `app.db`) rather than a hand-picked subset — a partial index set produces a misleading plan in either direction, not just "looks worse than it is." If it comes back unindexed for real, a `GROUP BY` aggregate is the usual fix. diff --git a/.gemini/skills/pr-analysis/SKILL.md b/.gemini/skills/pr-analysis/SKILL.md index 88442278a..edd32ba48 100644 --- a/.gemini/skills/pr-analysis/SKILL.md +++ b/.gemini/skills/pr-analysis/SKILL.md @@ -48,8 +48,8 @@ For each comment, determine: 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). +4. **Make changes one comment at a time** — keep commits focused. For a comment claiming a bug: write the test that should catch it first, run it against the current (unfixed) code, and confirm it fails for the right reason before writing the fix - see `prd-writing`'s test-first step for why (a test that never failed red can't be trusted to have caught anything). +5. **Rerun tests after each change** (`testing-workflow` skill) - confirm the new/updated test now passes, not just that nothing else broke. 6. **Reply** only after the commit is pushed. Include the short SHA. ## Reply Guidelines diff --git a/.gemini/skills/prd-writing/SKILL.md b/.gemini/skills/prd-writing/SKILL.md index 4f873f9cd..8d56a7389 100644 --- a/.gemini/skills/prd-writing/SKILL.md +++ b/.gemini/skills/prd-writing/SKILL.md @@ -26,7 +26,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 4. **For every mechanism, trace every downstream consumer — not just the first one you find.** The single highest-value question before calling a design complete: "where else does this exact same check or logic get independently re-derived?" In a codebase without one source of truth for a concept (e.g. "is this record currently active" computed by three different queries in three different files), patching the first occurrence and stopping is the most common way a design ships with a hidden, silent gap. Grep for the pattern, not just the function you already know about. 5. **Record rejected alternatives with the reasoning, not just the chosen design.** Give it its own subsection (`### Rejected: X`). Without this, a future reader — or your own future self — re-proposes the rejected idea because the "why not" only ever existed in a conversation, not in the document. 6. **Force every open question to an explicit decision**, even if the decision is "accept as-is for v1, revisit if feedback says otherwise." An open question left unresolved in a PRD gets silently decided by whoever implements it — usually differently than anyone intended. -7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). +7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason (a real assertion mismatch, not a collection/import error) before writing a line of the fix. A test that never failed red can't be trusted to have caught anything - it's usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. 8. **Ask explicitly whether validating this needs real end-to-end infrastructure** (a new or modified plugin, a UI click-through) or whether synthetic unit-level fixtures suffice — don't assume either way. Check whether the functions under test take a DB connection/dict/list as a parameter (testable in isolation, no real plugin needed) or require a real file on disk (harder to fake, may need one). 9. **Ask explicitly whether this feature/mechanism should be exposed via the API, and give a recommendation, not just flag it as an open question.** This codebase already has three real API surfaces to consider extending rather than inventing a fourth: REST (`server/api_server/*_endpoint.py`), GraphQL (`graphql_endpoint.py`), and a Prometheus `/metrics` endpoint (`prometheus_endpoint.py`). Skipping this question doesn't mean "no API access needed"; it means the answer gets silently decided later by whoever first wants to query the new data externally, usually as its own separate feature request re-litigating a design this PRD already had full context to settle. Not everything needs exposure: purely internal/diagnostic state with no plausible external consumer doesn't, but say so explicitly, with the reason, rather than leaving it unaddressed. 10. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` for *every* index on the tables involved, not just the first one you find — a column can have both a plain index and a separate expression index (e.g. `idx_eve_mac_date_type ON Events(eveMac, ...)` alongside `idx_eve_lower_mac_date_type ON Events(LOWER(eveMac), ...)`), and missing the second one produces a wrong verdict. Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count, built against the *complete* real index set (copy every `CREATE INDEX` for the table, or run it against an actual `app.db`) rather than a hand-picked subset — a partial index set produces a misleading plan in either direction, not just "looks worse than it is." If it comes back unindexed for real, a `GROUP BY` aggregate is the usual fix. diff --git a/.github/skills/pr-analysis/SKILL.md b/.github/skills/pr-analysis/SKILL.md index 6251ae55d..0a9c37da5 100644 --- a/.github/skills/pr-analysis/SKILL.md +++ b/.github/skills/pr-analysis/SKILL.md @@ -48,8 +48,8 @@ For each comment, determine: 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). +4. **Make changes one comment at a time** — keep commits focused. For a comment claiming a bug: write the test that should catch it first, run it against the current (unfixed) code, and confirm it fails for the right reason before writing the fix - see `prd-writing`'s test-first step for why (a test that never failed red can't be trusted to have caught anything). +5. **Rerun tests after each change** (`testing-workflow` skill) - confirm the new/updated test now passes, not just that nothing else broke. 6. **Reply** only after the commit is pushed via `report_progress`. Include the short SHA. ## Reply Guidelines diff --git a/.github/skills/prd-writing/SKILL.md b/.github/skills/prd-writing/SKILL.md index 5c052e50d..18c6d6ba8 100644 --- a/.github/skills/prd-writing/SKILL.md +++ b/.github/skills/prd-writing/SKILL.md @@ -26,7 +26,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 4. **For every mechanism, trace every downstream consumer — not just the first one you find.** The single highest-value question before calling a design complete: "where else does this exact same check or logic get independently re-derived?" In a codebase without one source of truth for a concept (e.g. "is this record currently active" computed by three different queries in three different files), patching the first occurrence and stopping is the most common way a design ships with a hidden, silent gap. Grep for the pattern, not just the function you already know about. 5. **Record rejected alternatives with the reasoning, not just the chosen design.** Give it its own subsection (`### Rejected: X`). Without this, a future reader — or your own future self — re-proposes the rejected idea because the "why not" only ever existed in a conversation, not in the document. 6. **Force every open question to an explicit decision**, even if the decision is "accept as-is for v1, revisit if feedback says otherwise." An open question left unresolved in a PRD gets silently decided by whoever implements it — usually differently than anyone intended. -7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). +7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason (a real assertion mismatch, not a collection/import error) before writing a line of the fix. A test that never failed red can't be trusted to have caught anything - it's usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. 8. **Ask explicitly whether validating this needs real end-to-end infrastructure** (a new or modified plugin, a UI click-through) or whether synthetic unit-level fixtures suffice — don't assume either way. Check whether the functions under test take a DB connection/dict/list as a parameter (testable in isolation, no real plugin needed) or require a real file on disk (harder to fake, may need one). 9. **Ask explicitly whether this feature/mechanism should be exposed via the API, and give a recommendation, not just flag it as an open question.** This codebase already has three real API surfaces to consider extending rather than inventing a fourth: REST (`server/api_server/*_endpoint.py`), GraphQL (`graphql_endpoint.py`), and a Prometheus `/metrics` endpoint (`prometheus_endpoint.py`). Skipping this question doesn't mean "no API access needed"; it means the answer gets silently decided later by whoever first wants to query the new data externally, usually as its own separate feature request re-litigating a design this PRD already had full context to settle. Not everything needs exposure: purely internal/diagnostic state with no plausible external consumer doesn't, but say so explicitly, with the reason, rather than leaving it unaddressed. 10. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` for *every* index on the tables involved, not just the first one you find — a column can have both a plain index and a separate expression index (e.g. `idx_eve_mac_date_type ON Events(eveMac, ...)` alongside `idx_eve_lower_mac_date_type ON Events(LOWER(eveMac), ...)`), and missing the second one produces a wrong verdict. Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count, built against the *complete* real index set (copy every `CREATE INDEX` for the table, or run it against an actual `app.db`) rather than a hand-picked subset — a partial index set produces a misleading plan in either direction, not just "looks worse than it is." If it comes back unindexed for real, a `GROUP BY` aggregate is the usual fix. From ebf12bfb6622c64d75f35ed31635697c74e11fa7 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Thu, 1 Oct 2026 20:40:26 +1000 Subject: [PATCH 04/13] DOCS: readme cleanup --- README.md | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index ecf2b2274..a93d8609b 100755 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ Use NetAlertX to spot shadow IT, unauthorized hardware, IPAM drift, and other ch ## Quick Start > [!WARNING] -> ⚠️ **Important:** The docker-compose has recently changed. Carefully read the [Migration guide](https://docs.netalertx.com/MIGRATION/?h=migrat#12-migration-from-netalertx-v25524) for detailed instructions. +> **Important:** If upgrading an older installation read the [Migration guide](https://docs.netalertx.com/MIGRATION/?h=migrat#12-migration-from-netalertx-v25524) for detailed instructions. Start NetAlertX in seconds with Docker: @@ -172,13 +172,13 @@ Check the [GitHub Issues](https://github.com/netalertx/NetAlertX/issues) for the jokob-sk%2FNetAlertX | Trendshift -### 📧 Get notified what's new +### Get notified what's new Get notified about a new release, what new functionality you can use and about breaking changes. ![Follow and star][follow_star] -### 🔀 Other Alternative Apps +### Other Alternative Apps - [Fing](https://www.fing.com/) - Network scanner app for your Internet security (Commercial, Phone App, Proprietary hardware) - [NetBox](https://netboxlabs.com/) - The gold standard for Network Source of Truth (NSoT) and IPAM. @@ -186,31 +186,13 @@ Get notified about a new release, what new functionality you can use and about b - [Domotz](https://www.domotz.com/) - Commercial network monitoring and remote management platform aimed at MSPs, IT teams, and multi-site environments. - [NetAlertX](https://netalertx.com) - The streamlined, discovery-focused choice for real-time asset intelligence and noise-free alerting. -### 💙 Donations - -Thank you to everyone who appreciates this tool and donates. - -
- Click for more ways to donate - -
- - | [![GitHub](https://i.imgur.com/emsRCPh.png)](https://github.com/sponsors/jokob-sk) | [![Buy Me A Coffee](https://i.imgur.com/pIM6YXL.png)](https://www.buymeacoffee.com/jokobsk) | - | --- | --- | - - Bitcoin: `1N8tupjeCK12qRVU2XrV17WvKK7LCawyZM` - - Ethereum: `0x6e2749Cb42F4411bc98501406BdcD82244e3f9C7` - - 📧 Email me at [support@netalertx.com](mailto:support@netalertx.com?subject=NetAlertX) if you want to get in touch or if I should add other sponsorship platforms. - -
- ### 🏗 Contributors This project would be nothing without the amazing work of the community, with special thanks to: > [pucherot/Pi.Alert](https://github.com/pucherot/Pi.Alert) (the original creator of PiAlert), [leiweibau](https://github.com/leiweibau/Pi.Alert): Dark mode (and much more), [Macleykun](https://github.com/Macleykun) (Help with Dockerfile clean-up), [vladaurosh](https://github.com/vladaurosh) for Alpine re-base help, [Final-Hawk](https://github.com/Final-Hawk) (Help with NTFY, styling and other fixes), [TeroRERO](https://github.com/terorero) (Spanish translations), [Data-Monkey](https://github.com/Data-Monkey), (Split-up of the python.py file and more), [cvc90](https://github.com/cvc90) (Spanish translation and various UI work) to name a few. Check out all the [amazing contributors](https://github.com/netalertx/NetAlertX/graphs/contributors). -### 🌍 Translations +### Translations Proudly using [Weblate](https://hosted.weblate.org/projects/pialert/). Help out and suggest languages in the [online portal of Weblate](https://hosted.weblate.org/projects/pialert/core/). From da6fbbbcb8ee42d2c6f76e4c33d838f310d64d57 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Fri, 2 Oct 2026 22:08:48 +1000 Subject: [PATCH 05/13] FE: timestamp fix #1826 --- front/js/devices-table.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/front/js/devices-table.js b/front/js/devices-table.js index c46565d73..bf58b2ad0 100644 --- a/front/js/devices-table.js +++ b/front/js/devices-table.js @@ -498,10 +498,11 @@ function initializeDatatable (status) { // Dates {targets: [mapIndx(COL.devFirstConnection), mapIndx(COL.devLastConnection)], 'createdCell': function (td, cellData, rowData, row, col) { - var result = cellData.toString(); // Convert to string - if (result.includes("+")) { // Check if timezone offset is present - result = result.split('+')[0]; // Remove timezone offset - } + // devFirstConnection/devLastConnection are DB NOT NULL with no default, + // but that still permits an empty string (e.g. stale rows from an older + // schema/version) - skip localizeTimestamp() for that case instead of + // showing its "Failed conversion" fallback for what is really just "no value". + var result = isEmpty(cellData) ? '' : localizeTimestamp(cellData); $(td).html (translateHTMLcodes (result)); } }, From 729e9cc481c5a0e15b70136056a0f2595bb77db5 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Fri, 2 Oct 2026 22:08:56 +1000 Subject: [PATCH 06/13] FE: timestamp fix #1826 --- front/php/templates/language/en_us.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/front/php/templates/language/en_us.json b/front/php/templates/language/en_us.json index 5776b9c2a..93d1e7422 100755 --- a/front/php/templates/language/en_us.json +++ b/front/php/templates/language/en_us.json @@ -240,7 +240,7 @@ "Device_TableHead_CustomProps": "Props / Actions", "Device_TableHead_FQDN": "FQDN", "Device_TableHead_Favorite": "Favorite", - "Device_TableHead_FirstSession": "First Session", + "Device_TableHead_FirstSession": "First Seen", "Device_TableHead_Flapping": "Flapping", "Device_TableHead_GUID": "GUID", "Device_TableHead_Group": "Group", @@ -249,7 +249,7 @@ "Device_TableHead_Icon": "Icon", "Device_TableHead_LastIP": "Last IP", "Device_TableHead_LastIPOrder": "Last IP Order", - "Device_TableHead_LastSession": "Last Offline", + "Device_TableHead_LastSession": "Last Seen", "Device_TableHead_Location": "Location", "Device_TableHead_MAC": "Random MAC", "Device_TableHead_MAC_full": "Full MAC", From af79016ac8159f1dce53d8c6c05d0f0428e03a69 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Fri, 2 Oct 2026 22:44:11 +1000 Subject: [PATCH 07/13] PLG: FREEBOX presence L3 fix #1828 --- server/plugins/freebox/freebox.py | 74 +++++++---- test/plugins/test_freebox.py | 201 ++++++++++++++++++++++++++++++ 2 files changed, 253 insertions(+), 22 deletions(-) create mode 100644 test/plugins/test_freebox.py diff --git a/server/plugins/freebox/freebox.py b/server/plugins/freebox/freebox.py index ab60a60a1..14a84c0a9 100755 --- a/server/plugins/freebox/freebox.py +++ b/server/plugins/freebox/freebox.py @@ -82,6 +82,40 @@ def map_device_type(type: str): return device_type_map["other"] +def select_l3_entries_for_presence(host): + """ + Decide which l3connectivities entries represent this host being present + this cycle. + + Normally one entry per currently-reachable L3 address (today's existing + behavior, e.g. both IPv4 and IPv6 reachable at once). When the host is + still active but none of its L3 addresses answer as reachable right now, + falls back to a single best-effort entry instead of reporting nothing - + a transient L3 reachability drop on every address must not be read as + "device gone" when the host-level `active` flag (the Freebox's own + traffic-based presence signal, independent of L3) says otherwise. See + GitHub issue #1828. Returns an empty list only when the host itself is + not active, which still correctly represents a genuinely absent device. + """ + l3 = host.get("l3connectivities") + if not isinstance(l3, list): + l3 = [] + + reachable = [ip for ip in l3 if ip.get("reachable")] + if reachable: + return reachable + + # Default True if the API unexpectedly omits "active", so a schema + # surprise fails open instead of silently reintroducing the #1828 bug. + if not host.get("active", True): + return [] + + mylog("verbose", [f"[{pluginName}] Host active but no reachable L3 address - using fallback IP"]) + if l3: + return [l3[0]] + return [{"addr": "0.0.0.0", "last_time_reachable": None}] + + async def get_device_data(api_version: int, api_address: str, api_port: int): # ensure existence of db path data_dir = Path(os.getenv("NETALERTX_CONFIG", "/data/config")) / "freeboxdb" @@ -159,28 +193,24 @@ def main(): foreignKey=freebox["mac"], ) for host in hosts: - # Check if 'l3connectivities' exists and is a list - if "l3connectivities" in host and isinstance(host["l3connectivities"], list): - for ip in [ip for ip in host["l3connectivities"] if ip.get("reachable")]: - mac: str = host.get("l2ident", {}).get("id", "(unknown)") - if mac != '(unknown)': - plugin_objects.add_object( - primaryId=mac, - secondaryId=ip.get("addr", "0.0.0.0"), - watched1=host.get("primary_name", "(unknown)"), - watched2=host.get("vendor_name", "(unknown)"), - watched3=map_device_type(host.get("host_type", "")), - # .get(..., 0) alone isn't enough: the Freebox API can return this - # key present but explicitly null, and dict.get()'s default only - # applies when the key is absent, not when its value is None - - # `or 0` catches both, avoiding a TypeError from fromtimestamp(None). - watched4=datetime.fromtimestamp(ip.get("last_time_reachable") or 0, tz=dt_timezone.utc).strftime(DATETIME_PATTERN), - extra="", - foreignKey=mac, - ) - else: - # Optional: Log or handle hosts without 'l3connectivities' - mylog("verbose", [f"[{pluginName}] Host missing 'l3connectivities': {host}"]) + mac: str = host.get("l2ident", {}).get("id", "(unknown)") + if mac == '(unknown)': + continue + for ip in select_l3_entries_for_presence(host): + plugin_objects.add_object( + primaryId=mac, + secondaryId=ip.get("addr", "0.0.0.0"), + watched1=host.get("primary_name", "(unknown)"), + watched2=host.get("vendor_name", "(unknown)"), + watched3=map_device_type(host.get("host_type", "")), + # .get(..., 0) alone isn't enough: the Freebox API can return this + # key present but explicitly null, and dict.get()'s default only + # applies when the key is absent, not when its value is None - + # `or 0` catches both, avoiding a TypeError from fromtimestamp(None). + watched4=datetime.fromtimestamp(ip.get("last_time_reachable") or 0, tz=dt_timezone.utc).strftime(DATETIME_PATTERN), + extra="", + foreignKey=mac, + ) # Commit result plugin_objects.write_result_file() diff --git a/test/plugins/test_freebox.py b/test/plugins/test_freebox.py new file mode 100644 index 000000000..a5180e02b --- /dev/null +++ b/test/plugins/test_freebox.py @@ -0,0 +1,201 @@ +""" +Tests for Freebox plugin (freebox.py). + +freebox.py is imported directly. Its module-level side effects +(get_setting_value, Logger, Plugin_Objects) are patched out before the +first import so no live config reads, log files, or result files are +created during tests. +""" + +import sys +import os +from unittest.mock import patch, MagicMock, AsyncMock + +# --------------------------------------------------------------------------- +# Path setup +# --------------------------------------------------------------------------- + +_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..")) +_SERVER = os.path.join(_ROOT, "server") +_PLUGINS = os.path.join(_ROOT, "server", "plugins") +_PLUGIN_DIR = os.path.join(_ROOT, "server", "plugins", "freebox") + +for _p in [_ROOT, _SERVER, _PLUGINS, _PLUGIN_DIR]: + if _p not in sys.path: + sys.path.insert(0, _p) + +# --------------------------------------------------------------------------- +# Import freebox with module-level side effects patched +# --------------------------------------------------------------------------- +# freebox.py calls get_setting_value(), Logger(), and Plugin_Objects() at +# module level. Patching these before the first import prevents live config +# reads, log-file creation, and result-file creation during tests. + +with patch("helper.get_setting_value", return_value="UTC"), \ + patch("logger.Logger"), \ + patch("plugin_helper.Plugin_Objects"): + import freebox # noqa: E402 + + +# --------------------------------------------------------------------------- +# Shared helpers +# --------------------------------------------------------------------------- + +def _l3(addr="192.168.1.10", reachable=True, last_time_reachable=1700000000): + return {"addr": addr, "reachable": reachable, "last_time_reachable": last_time_reachable} + + +def _host(mac="aa:bb:cc:dd:ee:01", active=True, l3connectivities=None, + name="testdevice", vendor="TestVendor", host_type="workstation"): + host = { + "l2ident": {"id": mac}, + "primary_name": name, + "vendor_name": vendor, + "host_type": host_type, + } + if active is not None: + host["active"] = active + if l3connectivities is not None: + host["l3connectivities"] = l3connectivities + return host + + +# =========================================================================== +# select_l3_entries_for_presence - pure decision function +# =========================================================================== + +class TestSelectL3EntriesForPresence: + + def test_prefers_reachable_entries_when_available(self): + l3_entries = [_l3("10.0.0.1", reachable=True), _l3("10.0.0.2", reachable=False)] + host = _host(l3connectivities=l3_entries) + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["10.0.0.1"] + + def test_returns_all_reachable_entries_unchanged(self): + """Today's existing multi-IP-per-device behavior (e.g. IPv4 + IPv6 + both reachable) must be preserved exactly - one row per reachable + address, not collapsed to a single fallback.""" + l3_entries = [_l3("10.0.0.1", reachable=True), _l3("fe80::1", reachable=True)] + host = _host(l3connectivities=l3_entries) + result = freebox.select_l3_entries_for_presence(host) + assert {e["addr"] for e in result} == {"10.0.0.1", "fe80::1"} + + def test_falls_back_to_unreachable_entry_when_host_active(self): + """The exact bug from issue #1828: host.active=True but every L3 + address reports reachable=False must still report presence, using + the best-available (even if currently unreachable) address.""" + host = _host(active=True, l3connectivities=[_l3("10.0.0.1", reachable=False)]) + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["10.0.0.1"] + + def test_returns_empty_when_host_not_active(self): + """A genuinely absent host (active=False) must still be skipped - + this fallback must not mask a real disconnection.""" + host = _host(active=False, l3connectivities=[_l3("10.0.0.1", reachable=False)]) + result = freebox.select_l3_entries_for_presence(host) + assert result == [] + + def test_missing_active_key_defaults_to_present(self): + """Fail open if the API unexpectedly omits 'active', rather than + silently reintroducing the false-disconnect bug this exists to fix.""" + host = _host(active=None, l3connectivities=[_l3("10.0.0.1", reachable=False)]) + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["10.0.0.1"] + + def test_sentinel_ip_when_active_but_no_l3_entries_at_all(self): + host = _host(active=True, l3connectivities=[]) + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["0.0.0.0"] + + def test_sentinel_ip_when_l3connectivities_missing_entirely(self): + host = _host(active=True) # l3connectivities key omitted entirely + assert "l3connectivities" not in host + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["0.0.0.0"] + + def test_non_list_l3connectivities_treated_as_absent(self): + host = _host(active=True) + host["l3connectivities"] = "not-a-list" + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["0.0.0.0"] + + +# =========================================================================== +# main() - end-to-end row emission +# =========================================================================== + +class TestMainHostLoop: + + _SETTINGS = { + "FREEBOX_address": "mafreebox.freebox.fr", + "FREEBOX_api_version": 6, + "FREEBOX_api_port": 443, + } + + def _patch_settings(self): + return patch.object(freebox, "get_setting_value", side_effect=lambda k: self._SETTINGS[k]) + + def test_reporter_scenario_active_but_unreachable_still_emits(self): + """Regression for issue #1828: a host with active=True but every L3 + address reachable=False must still get a row emitted, not be + silently dropped (which the scan pipeline would otherwise read as + 'device gone' and fire a false Disconnected/Flapping event).""" + hosts = [_host(mac="aa:bb:cc:dd:ee:01", active=True, + l3connectivities=[_l3("10.0.0.1", reachable=False)])] + mock_po = MagicMock() + + with self._patch_settings(), \ + patch.object(freebox, "get_device_data", AsyncMock(return_value=(None, hosts))), \ + patch.object(freebox, "plugin_objects", mock_po): + result = freebox.main() + + assert result == 0 + assert mock_po.add_object.call_count == 1 + call = mock_po.add_object.call_args_list[0] + assert call.kwargs["primaryId"] == "aa:bb:cc:dd:ee:01" + assert call.kwargs["secondaryId"] == "10.0.0.1" + + def test_inactive_host_still_not_emitted(self): + """Regression guard: a genuinely absent host must not start being + reported as present as a side effect of fixing #1828.""" + hosts = [_host(mac="aa:bb:cc:dd:ee:02", active=False, + l3connectivities=[_l3("10.0.0.2", reachable=False)])] + mock_po = MagicMock() + + with self._patch_settings(), \ + patch.object(freebox, "get_device_data", AsyncMock(return_value=(None, hosts))), \ + patch.object(freebox, "plugin_objects", mock_po): + result = freebox.main() + + assert result == 0 + assert mock_po.add_object.call_count == 0 + + def test_reachable_host_unchanged(self): + """Regression guard: the common/working case (at least one reachable + L3 address) must be unaffected by this fix.""" + hosts = [_host(mac="aa:bb:cc:dd:ee:03", active=True, + l3connectivities=[_l3("10.0.0.3", reachable=True)])] + mock_po = MagicMock() + + with self._patch_settings(), \ + patch.object(freebox, "get_device_data", AsyncMock(return_value=(None, hosts))), \ + patch.object(freebox, "plugin_objects", mock_po): + result = freebox.main() + + assert result == 0 + assert mock_po.add_object.call_count == 1 + assert mock_po.add_object.call_args_list[0].kwargs["secondaryId"] == "10.0.0.3" + + def test_unknown_mac_still_skipped(self): + host = _host(active=True, l3connectivities=[_l3("10.0.0.4", reachable=False)]) + host["l2ident"] = {} + mock_po = MagicMock() + + with self._patch_settings(), \ + patch.object(freebox, "get_device_data", AsyncMock(return_value=(None, [host]))), \ + patch.object(freebox, "plugin_objects", mock_po): + result = freebox.main() + + assert result == 0 + assert mock_po.add_object.call_count == 0 From 067a6a15ef99a97561bec3a7e5c798d5751f9df1 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Fri, 2 Oct 2026 23:05:02 +1000 Subject: [PATCH 08/13] FE+BE: review fixes #1828 #1826 --- .claude/skills/prd-writing/SKILL.md | 2 +- .gemini/skills/prd-writing/SKILL.md | 2 +- .github/skills/prd-writing/SKILL.md | 2 +- README.md | 2 +- front/js/devices-table.js | 5 +++ server/plugins/freebox/freebox.py | 49 +++++++++++++---------- test/plugins/test_freebox.py | 61 +++++++++++++++++++++++++---- 7 files changed, 90 insertions(+), 33 deletions(-) diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md index 1cca925d8..08a539fea 100644 --- a/.claude/skills/prd-writing/SKILL.md +++ b/.claude/skills/prd-writing/SKILL.md @@ -26,7 +26,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 4. **For every mechanism, trace every downstream consumer — not just the first one you find.** The single highest-value question before calling a design complete: "where else does this exact same check or logic get independently re-derived?" In a codebase without one source of truth for a concept (e.g. "is this record currently active" computed by three different queries in three different files), patching the first occurrence and stopping is the most common way a design ships with a hidden, silent gap. Grep for the pattern, not just the function you already know about. 5. **Record rejected alternatives with the reasoning, not just the chosen design.** Give it its own subsection (`### Rejected: X`). Without this, a future reader — or your own future self — re-proposes the rejected idea because the "why not" only ever existed in a conversation, not in the document. 6. **Force every open question to an explicit decision**, even if the decision is "accept as-is for v1, revisit if feedback says otherwise." An open question left unresolved in a PRD gets silently decided by whoever implements it — usually differently than anyone intended. -7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason (a real assertion mismatch, not a collection/import error) before writing a line of the fix. A test that never failed red can't be trusted to have caught anything - it's usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. +7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason before writing a line of the fix. What "the right reason" means depends on what's being tested: a regression test against existing behavior must fail with a real behavioral assertion mismatch, not a collection/import error - a test that never failed red that way can't be trusted to have caught anything, and is usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). A test for a brand-new contract (a function/interface that doesn't exist yet) legitimately fails with a missing-interface error instead (`AttributeError`, `ImportError`) before it's written - that's the expected red for that case, not a sign the test is wrong. Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. 8. **Ask explicitly whether validating this needs real end-to-end infrastructure** (a new or modified plugin, a UI click-through) or whether synthetic unit-level fixtures suffice — don't assume either way. Check whether the functions under test take a DB connection/dict/list as a parameter (testable in isolation, no real plugin needed) or require a real file on disk (harder to fake, may need one). 9. **Ask explicitly whether this feature/mechanism should be exposed via the API, and give a recommendation, not just flag it as an open question.** This codebase already has three real API surfaces to consider extending rather than inventing a fourth: REST (`server/api_server/*_endpoint.py`), GraphQL (`graphql_endpoint.py`), and a Prometheus `/metrics` endpoint (`prometheus_endpoint.py`). Skipping this question doesn't mean "no API access needed"; it means the answer gets silently decided later by whoever first wants to query the new data externally, usually as its own separate feature request re-litigating a design this PRD already had full context to settle. Not everything needs exposure: purely internal/diagnostic state with no plausible external consumer doesn't, but say so explicitly, with the reason, rather than leaving it unaddressed. 10. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` for *every* index on the tables involved, not just the first one you find — a column can have both a plain index and a separate expression index (e.g. `idx_eve_mac_date_type ON Events(eveMac, ...)` alongside `idx_eve_lower_mac_date_type ON Events(LOWER(eveMac), ...)`), and missing the second one produces a wrong verdict. Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count, built against the *complete* real index set (copy every `CREATE INDEX` for the table, or run it against an actual `app.db`) rather than a hand-picked subset — a partial index set produces a misleading plan in either direction, not just "looks worse than it is." If it comes back unindexed for real, a `GROUP BY` aggregate is the usual fix. diff --git a/.gemini/skills/prd-writing/SKILL.md b/.gemini/skills/prd-writing/SKILL.md index 8d56a7389..41fdb1896 100644 --- a/.gemini/skills/prd-writing/SKILL.md +++ b/.gemini/skills/prd-writing/SKILL.md @@ -26,7 +26,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 4. **For every mechanism, trace every downstream consumer — not just the first one you find.** The single highest-value question before calling a design complete: "where else does this exact same check or logic get independently re-derived?" In a codebase without one source of truth for a concept (e.g. "is this record currently active" computed by three different queries in three different files), patching the first occurrence and stopping is the most common way a design ships with a hidden, silent gap. Grep for the pattern, not just the function you already know about. 5. **Record rejected alternatives with the reasoning, not just the chosen design.** Give it its own subsection (`### Rejected: X`). Without this, a future reader — or your own future self — re-proposes the rejected idea because the "why not" only ever existed in a conversation, not in the document. 6. **Force every open question to an explicit decision**, even if the decision is "accept as-is for v1, revisit if feedback says otherwise." An open question left unresolved in a PRD gets silently decided by whoever implements it — usually differently than anyone intended. -7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason (a real assertion mismatch, not a collection/import error) before writing a line of the fix. A test that never failed red can't be trusted to have caught anything - it's usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. +7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason before writing a line of the fix. What "the right reason" means depends on what's being tested: a regression test against existing behavior must fail with a real behavioral assertion mismatch, not a collection/import error - a test that never failed red that way can't be trusted to have caught anything, and is usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). A test for a brand-new contract (a function/interface that doesn't exist yet) legitimately fails with a missing-interface error instead (`AttributeError`, `ImportError`) before it's written - that's the expected red for that case, not a sign the test is wrong. Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. 8. **Ask explicitly whether validating this needs real end-to-end infrastructure** (a new or modified plugin, a UI click-through) or whether synthetic unit-level fixtures suffice — don't assume either way. Check whether the functions under test take a DB connection/dict/list as a parameter (testable in isolation, no real plugin needed) or require a real file on disk (harder to fake, may need one). 9. **Ask explicitly whether this feature/mechanism should be exposed via the API, and give a recommendation, not just flag it as an open question.** This codebase already has three real API surfaces to consider extending rather than inventing a fourth: REST (`server/api_server/*_endpoint.py`), GraphQL (`graphql_endpoint.py`), and a Prometheus `/metrics` endpoint (`prometheus_endpoint.py`). Skipping this question doesn't mean "no API access needed"; it means the answer gets silently decided later by whoever first wants to query the new data externally, usually as its own separate feature request re-litigating a design this PRD already had full context to settle. Not everything needs exposure: purely internal/diagnostic state with no plausible external consumer doesn't, but say so explicitly, with the reason, rather than leaving it unaddressed. 10. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` for *every* index on the tables involved, not just the first one you find — a column can have both a plain index and a separate expression index (e.g. `idx_eve_mac_date_type ON Events(eveMac, ...)` alongside `idx_eve_lower_mac_date_type ON Events(LOWER(eveMac), ...)`), and missing the second one produces a wrong verdict. Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count, built against the *complete* real index set (copy every `CREATE INDEX` for the table, or run it against an actual `app.db`) rather than a hand-picked subset — a partial index set produces a misleading plan in either direction, not just "looks worse than it is." If it comes back unindexed for real, a `GROUP BY` aggregate is the usual fix. diff --git a/.github/skills/prd-writing/SKILL.md b/.github/skills/prd-writing/SKILL.md index 18c6d6ba8..c0fbdbe76 100644 --- a/.github/skills/prd-writing/SKILL.md +++ b/.github/skills/prd-writing/SKILL.md @@ -26,7 +26,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 4. **For every mechanism, trace every downstream consumer — not just the first one you find.** The single highest-value question before calling a design complete: "where else does this exact same check or logic get independently re-derived?" In a codebase without one source of truth for a concept (e.g. "is this record currently active" computed by three different queries in three different files), patching the first occurrence and stopping is the most common way a design ships with a hidden, silent gap. Grep for the pattern, not just the function you already know about. 5. **Record rejected alternatives with the reasoning, not just the chosen design.** Give it its own subsection (`### Rejected: X`). Without this, a future reader — or your own future self — re-proposes the rejected idea because the "why not" only ever existed in a conversation, not in the document. 6. **Force every open question to an explicit decision**, even if the decision is "accept as-is for v1, revisit if feedback says otherwise." An open question left unresolved in a PRD gets silently decided by whoever implements it — usually differently than anyone intended. -7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason (a real assertion mismatch, not a collection/import error) before writing a line of the fix. A test that never failed red can't be trusted to have caught anything - it's usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. +7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure). When execution actually starts, write that test code before the implementation, run it against the pre-fix code, and confirm it fails for the right reason before writing a line of the fix. What "the right reason" means depends on what's being tested: a regression test against existing behavior must fail with a real behavioral assertion mismatch, not a collection/import error - a test that never failed red that way can't be trusted to have caught anything, and is usually the first sign the fixture doesn't actually distinguish broken from fixed behavior (a real case: a single-device DB fixture passed against both the buggy and the fixed code, because nothing in it could tell "this row matches" from "any row matches" - see `scan-pipeline` Gotcha 8). A test for a brand-new contract (a function/interface that doesn't exist yet) legitimately fails with a missing-interface error instead (`AttributeError`, `ImportError`) before it's written - that's the expected red for that case, not a sign the test is wrong. Writing the test first also tends to surface implementation code that's awkward to exercise in isolation - treat that as a refactor signal, not friction to route around. 8. **Ask explicitly whether validating this needs real end-to-end infrastructure** (a new or modified plugin, a UI click-through) or whether synthetic unit-level fixtures suffice — don't assume either way. Check whether the functions under test take a DB connection/dict/list as a parameter (testable in isolation, no real plugin needed) or require a real file on disk (harder to fake, may need one). 9. **Ask explicitly whether this feature/mechanism should be exposed via the API, and give a recommendation, not just flag it as an open question.** This codebase already has three real API surfaces to consider extending rather than inventing a fourth: REST (`server/api_server/*_endpoint.py`), GraphQL (`graphql_endpoint.py`), and a Prometheus `/metrics` endpoint (`prometheus_endpoint.py`). Skipping this question doesn't mean "no API access needed"; it means the answer gets silently decided later by whoever first wants to query the new data externally, usually as its own separate feature request re-litigating a design this PRD already had full context to settle. Not everything needs exposure: purely internal/diagnostic state with no plausible external consumer doesn't, but say so explicitly, with the reason, rather than leaving it unaddressed. 10. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` for *every* index on the tables involved, not just the first one you find — a column can have both a plain index and a separate expression index (e.g. `idx_eve_mac_date_type ON Events(eveMac, ...)` alongside `idx_eve_lower_mac_date_type ON Events(LOWER(eveMac), ...)`), and missing the second one produces a wrong verdict. Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count, built against the *complete* real index set (copy every `CREATE INDEX` for the table, or run it against an actual `app.db`) rather than a hand-picked subset — a partial index set produces a misleading plan in either direction, not just "looks worse than it is." If it comes back unindexed for real, a `GROUP BY` aggregate is the usual fix. diff --git a/README.md b/README.md index a93d8609b..e2bac24c3 100755 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ Use NetAlertX to spot shadow IT, unauthorized hardware, IPAM drift, and other ch ## Quick Start > [!WARNING] -> **Important:** If upgrading an older installation read the [Migration guide](https://docs.netalertx.com/MIGRATION/?h=migrat#12-migration-from-netalertx-v25524) for detailed instructions. +> **Important:** If upgrading an older installation read the [Migration guide](https://docs.netalertx.com/MIGRATION/) for detailed instructions - it lists each migration scenario by version, so pick the one matching your installed version. Start NetAlertX in seconds with Docker: diff --git a/front/js/devices-table.js b/front/js/devices-table.js index bf58b2ad0..55253c00e 100644 --- a/front/js/devices-table.js +++ b/front/js/devices-table.js @@ -496,6 +496,11 @@ function initializeDatatable (status) { } }, // Dates + /** + * Renders the First Connection / Last Offline column cells: an empty + * cellData renders as a blank cell, otherwise as cellData localized + * into the user's configured timezone/locale. + */ {targets: [mapIndx(COL.devFirstConnection), mapIndx(COL.devLastConnection)], 'createdCell': function (td, cellData, rowData, row, col) { // devFirstConnection/devLastConnection are DB NOT NULL with no default, diff --git a/server/plugins/freebox/freebox.py b/server/plugins/freebox/freebox.py index 14a84c0a9..34f153b3f 100755 --- a/server/plugins/freebox/freebox.py +++ b/server/plugins/freebox/freebox.py @@ -84,18 +84,12 @@ def map_device_type(type: str): def select_l3_entries_for_presence(host): """ - Decide which l3connectivities entries represent this host being present - this cycle. - - Normally one entry per currently-reachable L3 address (today's existing - behavior, e.g. both IPv4 and IPv6 reachable at once). When the host is - still active but none of its L3 addresses answer as reachable right now, - falls back to a single best-effort entry instead of reporting nothing - - a transient L3 reachability drop on every address must not be read as - "device gone" when the host-level `active` flag (the Freebox's own - traffic-based presence signal, independent of L3) says otherwise. See - GitHub issue #1828. Returns an empty list only when the host itself is - not active, which still correctly represents a genuinely absent device. + Select which l3connectivities entries represent presence for a host this + cycle: every currently-reachable entry if at least one exists; otherwise, + if the host itself is active, a single best-effort entry (preferring one + Freebox still marks active even though unreachable, else the first + entry, else an empty dict if there are no L3 entries at all); otherwise + (host not active) an empty list. """ l3 = host.get("l3connectivities") if not isinstance(l3, list): @@ -112,8 +106,15 @@ def select_l3_entries_for_presence(host): mylog("verbose", [f"[{pluginName}] Host active but no reachable L3 address - using fallback IP"]) if l3: - return [l3[0]] - return [{"addr": "0.0.0.0", "last_time_reachable": None}] + # Each l3connectivities entry has its own "active" flag, independent + # of "reachable" - prefer one Freebox still considers active over an + # arbitrary stale entry; fall back to the first entry if none are. + return [next((e for e in l3 if e.get("active")), l3[0])] + + # No L3 data at all for this host this cycle - still assert presence + # (primaryId/MAC alone is enough), but don't fabricate an address or + # timestamp. main() leaves secondaryId/watched4 blank for an empty dict. + return [{}] async def get_device_data(api_version: int, api_address: str, api_port: int): @@ -197,17 +198,23 @@ def main(): if mac == '(unknown)': continue for ip in select_l3_entries_for_presence(host): - plugin_objects.add_object( - primaryId=mac, - secondaryId=ip.get("addr", "0.0.0.0"), - watched1=host.get("primary_name", "(unknown)"), - watched2=host.get("vendor_name", "(unknown)"), - watched3=map_device_type(host.get("host_type", "")), + if "last_time_reachable" in ip: # .get(..., 0) alone isn't enough: the Freebox API can return this # key present but explicitly null, and dict.get()'s default only # applies when the key is absent, not when its value is None - # `or 0` catches both, avoiding a TypeError from fromtimestamp(None). - watched4=datetime.fromtimestamp(ip.get("last_time_reachable") or 0, tz=dt_timezone.utc).strftime(DATETIME_PATTERN), + watched4 = datetime.fromtimestamp(ip.get("last_time_reachable") or 0, tz=dt_timezone.utc).strftime(DATETIME_PATTERN) + else: + # select_l3_entries_for_presence()'s no-L3-data fallback ({}) - + # leave blank rather than fabricating an epoch-zero timestamp. + watched4 = "" + plugin_objects.add_object( + primaryId=mac, + secondaryId=ip.get("addr", ""), + watched1=host.get("primary_name", "(unknown)"), + watched2=host.get("vendor_name", "(unknown)"), + watched3=map_device_type(host.get("host_type", "")), + watched4=watched4, extra="", foreignKey=mac, ) diff --git a/test/plugins/test_freebox.py b/test/plugins/test_freebox.py index a5180e02b..59ee9dff2 100644 --- a/test/plugins/test_freebox.py +++ b/test/plugins/test_freebox.py @@ -41,8 +41,11 @@ with patch("helper.get_setting_value", return_value="UTC"), \ # Shared helpers # --------------------------------------------------------------------------- -def _l3(addr="192.168.1.10", reachable=True, last_time_reachable=1700000000): - return {"addr": addr, "reachable": reachable, "last_time_reachable": last_time_reachable} +def _l3(addr="192.168.1.10", reachable=True, active=None, last_time_reachable=1700000000): + entry = {"addr": addr, "reachable": reachable, "last_time_reachable": last_time_reachable} + if active is not None: + entry["active"] = active + return entry def _host(mac="aa:bb:cc:dd:ee:01", active=True, l3connectivities=None, @@ -103,22 +106,44 @@ class TestSelectL3EntriesForPresence: result = freebox.select_l3_entries_for_presence(host) assert [e["addr"] for e in result] == ["10.0.0.1"] - def test_sentinel_ip_when_active_but_no_l3_entries_at_all(self): + def test_prefers_active_entry_among_unreachable_when_available(self): + """Each l3connectivities entry has its own 'active' flag, independent + of 'reachable' (per the Freebox API's LanHostL3Connectivity schema) - + when nothing is reachable, an entry Freebox still marks active is a + better guess than an arbitrary stale one. The active entry is placed + second on purpose, so a naive "just take the first one" fallback + would fail this test.""" + l3_entries = [_l3("10.0.0.1", reachable=False, active=False), + _l3("10.0.0.2", reachable=False, active=True)] + host = _host(active=True, l3connectivities=l3_entries) + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["10.0.0.2"] + + def test_falls_back_to_first_entry_when_none_are_active_either(self): + l3_entries = [_l3("10.0.0.1", reachable=False), _l3("10.0.0.2", reachable=False)] + host = _host(active=True, l3connectivities=l3_entries) + result = freebox.select_l3_entries_for_presence(host) + assert [e["addr"] for e in result] == ["10.0.0.1"] + + def test_empty_entry_when_active_but_no_l3_entries_at_all(self): + """No fabricated '0.0.0.0' address or epoch-zero timestamp when there's + genuinely no L3 data - an empty dict lets main() leave secondaryId/ + watched4 blank instead of writing misleading placeholder values.""" host = _host(active=True, l3connectivities=[]) result = freebox.select_l3_entries_for_presence(host) - assert [e["addr"] for e in result] == ["0.0.0.0"] + assert result == [{}] - def test_sentinel_ip_when_l3connectivities_missing_entirely(self): + def test_empty_entry_when_l3connectivities_missing_entirely(self): host = _host(active=True) # l3connectivities key omitted entirely assert "l3connectivities" not in host result = freebox.select_l3_entries_for_presence(host) - assert [e["addr"] for e in result] == ["0.0.0.0"] + assert result == [{}] - def test_non_list_l3connectivities_treated_as_absent(self): + def test_empty_entry_when_l3connectivities_not_a_list(self): host = _host(active=True) host["l3connectivities"] = "not-a-list" result = freebox.select_l3_entries_for_presence(host) - assert [e["addr"] for e in result] == ["0.0.0.0"] + assert result == [{}] # =========================================================================== @@ -171,6 +196,26 @@ class TestMainHostLoop: assert result == 0 assert mock_po.add_object.call_count == 0 + def test_active_host_no_l3_entries_emits_blank_ip_and_timestamp(self): + """A host with active=True but no l3connectivities at all still gets + a presence row (primaryId/MAC alone is enough to assert presence), + but must not fabricate a '0.0.0.0' address or an epoch-zero + 'last seen' timestamp - both would be misleading for data we don't + actually have.""" + hosts = [_host(mac="aa:bb:cc:dd:ee:05", active=True, l3connectivities=[])] + mock_po = MagicMock() + + with self._patch_settings(), \ + patch.object(freebox, "get_device_data", AsyncMock(return_value=(None, hosts))), \ + patch.object(freebox, "plugin_objects", mock_po): + result = freebox.main() + + assert result == 0 + assert mock_po.add_object.call_count == 1 + call = mock_po.add_object.call_args_list[0] + assert call.kwargs["secondaryId"] == "" + assert call.kwargs["watched4"] == "" + def test_reachable_host_unchanged(self): """Regression guard: the common/working case (at least one reachable L3 address) must be unaffected by this fix.""" From 27aa36620e9eda5b0c683fd55be889181dfd3ba7 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 3 Oct 2026 09:01:21 +1000 Subject: [PATCH 09/13] FE+DOCS: review fixes --- CONTRIBUTING.md | 11 ++++++----- docs/PLUGINS_DEV.md | 1 + front/php/templates/security.php | 9 ++++++++- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a24532617..7bfd582ba 100755 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,7 +50,7 @@ All changes must pass the **full test suite** before opening a PR. ## Submitting Pull Requests (PRs) -We welcome PRs to improve the code, docs, or UI! +This project welcomes PRs to improve the code, docs, or UI! Please: - Ensure **backward compatibility** with existing installations @@ -58,6 +58,7 @@ Please: - Follow existing **code style and structure** - Provide a clear title and description for your PR - If relevant, add or update tests and documentation +- For a bug fix, write the test that reproduces it *before* the fix, confirm it fails, then fix it and confirm it passes - this is what actually proves the test catches the bug (see [testing workflow](/.github/skills/testing-workflow/SKILL.md)) - For plugins, refer to the [Plugin Dev Guide](https://docs.netalertx.com/PLUGINS_DEV) - Switch the PR to DRAFT mode if still being worked on - Keep PRs **focused and minimal** — avoid unrelated changes in a single PR @@ -79,19 +80,19 @@ Please: New to open source? Check out these resources: - [How to Fork and Submit a PR](https://opensource.guide/how-to-contribute/) -- Ask questions or get support in our [Discord](https://discord.gg/NczTUTWyRr) +- Ask questions or get support in [Discord](https://discord.gg/NczTUTWyRr) --- ## Code of Conduct -By participating, you agree to follow our [Code of Conduct](./CODE_OF_CONDUCT.md), which ensures a respectful and welcoming community. +By participating, you agree to follow the [Code of Conduct](./CODE_OF_CONDUCT.md), which ensures a respectful and welcoming community. --- ## Contact If you have more in-depth questions or want to discuss contributing in other ways, feel free to reach out at: -[jokob.sk@gmail.com](mailto:jokob.sk@gmail.com?subject=NetAlertX%20Contribution) +[support@netalertx.com](mailto:support@netalertx.com?subject=NetAlertX%20Contribution) -We appreciate every contribution, big or small! 💙 +Every contribution, big or small, is appreaciated! 💙 diff --git a/docs/PLUGINS_DEV.md b/docs/PLUGINS_DEV.md index 1ca1d8d00..cfce62b93 100755 --- a/docs/PLUGINS_DEV.md +++ b/docs/PLUGINS_DEV.md @@ -91,6 +91,7 @@ If you can imagine it and script it, you can build a plugin. 2. Test via Settings → Plugin Settings 3. Verify results in UI and logs 4. Check `/tmp/log/plugins/last_result..log` +5. Add unit tests under `test/plugins/` for any new or changed plugin logic - see an existing plugin's test file (e.g. `test_fritzbox.py`) for the pattern See [Quick Start Guide](PLUGINS_DEV_QUICK_START.md) for detailed step-by-step instructions. diff --git a/front/php/templates/security.php b/front/php/templates/security.php index a87e91e1a..605ac7cd3 100755 --- a/front/php/templates/security.php +++ b/front/php/templates/security.php @@ -28,7 +28,14 @@ function getConfigLine($pattern, $config_lines) { function getConfigValue($pattern, $config_lines, $delimiter = "'") { $line = preg_grep($pattern, $config_lines); - return !empty($line) ? explode($delimiter, array_values($line)[0])[1] : ''; + if (empty($line)) { + return ''; + } + // encode_python_string() (front/php/server/util.php) doubles backslashes + // before writing to app.conf so they round-trip through the Python-style + // single-quoted literal unchanged - undo that here, or a password/token + // containing a literal backslash never compares equal to what was saved. + return str_replace('\\\\', '\\', explode($delimiter, array_values($line)[0])[1]); } function redirect($url) { From a72807436af87cf25d0f072fff4c1a007e8ab3b0 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 3 Oct 2026 09:23:44 +1000 Subject: [PATCH 10/13] DOCS: review fixes --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7bfd582ba..80d571dd6 100755 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -95,4 +95,4 @@ By participating, you agree to follow the [Code of Conduct](./CODE_OF_CONDUCT.md If you have more in-depth questions or want to discuss contributing in other ways, feel free to reach out at: [support@netalertx.com](mailto:support@netalertx.com?subject=NetAlertX%20Contribution) -Every contribution, big or small, is appreaciated! 💙 +Every contribution, big or small, is appreciated! 💙 From 61e6d81fc3e89e002c50161b6741100034d4af37 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 3 Oct 2026 11:00:11 +1000 Subject: [PATCH 11/13] FE+DOCS: settings pending changes indicator --- .claude/skills/pr-analysis/SKILL.md | 2 +- .claude/skills/prd-writing/SKILL.md | 2 ++ .gemini/skills/pr-analysis/SKILL.md | 2 +- .gemini/skills/prd-writing/SKILL.md | 2 ++ .github/skills/code-standards/SKILL.md | 25 ++++++++++++++++++ .github/skills/pr-analysis/SKILL.md | 2 +- .github/skills/prd-writing/SKILL.md | 2 ++ CLAUDE.md | 4 +++ front/css/app.css | 20 ++++++++++++++ front/js/common.js | 35 +++++++++++++++++++++++++ front/js/handle_pending_settings.js | 14 ++++++++++ front/js/handle_version.js | 10 ++++--- front/js/sse_manager.js | 8 ++++++ front/php/templates/footer.php | 3 ++- front/php/templates/header.php | 8 ++++++ front/php/templates/language/ar_ar.json | 2 ++ front/php/templates/language/ca_ca.json | 4 ++- front/php/templates/language/cs_cz.json | 2 ++ front/php/templates/language/de_de.json | 4 ++- front/php/templates/language/en_us.json | 2 ++ front/php/templates/language/es_es.json | 4 ++- front/php/templates/language/fa_fa.json | 2 ++ front/php/templates/language/fi_fi.json | 2 ++ front/php/templates/language/fr_fr.json | 4 ++- front/php/templates/language/he_il.json | 2 ++ front/php/templates/language/hu_hu.json | 2 ++ front/php/templates/language/id_id.json | 2 ++ front/php/templates/language/it_it.json | 4 ++- front/php/templates/language/ja_jp.json | 2 ++ front/php/templates/language/nb_no.json | 2 ++ front/php/templates/language/pl_pl.json | 2 ++ front/php/templates/language/pt_br.json | 2 ++ front/php/templates/language/pt_pt.json | 2 ++ front/php/templates/language/ru_ru.json | 2 ++ front/php/templates/language/sv_sv.json | 2 ++ front/php/templates/language/tr_tr.json | 2 ++ front/php/templates/language/uk_ua.json | 2 ++ front/php/templates/language/vi_vn.json | 2 ++ front/php/templates/language/zh_cn.json | 2 ++ front/settings.php | 16 ++++++++++- 40 files changed, 199 insertions(+), 14 deletions(-) create mode 100644 front/js/handle_pending_settings.js diff --git a/.claude/skills/pr-analysis/SKILL.md b/.claude/skills/pr-analysis/SKILL.md index 3a17b8a88..1f97474d7 100644 --- a/.claude/skills/pr-analysis/SKILL.md +++ b/.claude/skills/pr-analysis/SKILL.md @@ -47,7 +47,7 @@ For each comment, determine: 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. +3. **Prepare a plan** — list each file and the exact change required. If a comment calls for new logic (a new check, helper, or condition), search for an existing equivalent first - the whole file being edited, not just the section in question, plus sibling pages/the Python backend - and extract/reuse it rather than planning a parallel implementation (see `code-standards`' DRY Principle section). 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. diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md index 08a539fea..85facbd2f 100644 --- a/.claude/skills/prd-writing/SKILL.md +++ b/.claude/skills/prd-writing/SKILL.md @@ -9,6 +9,8 @@ description: Read before writing a PRD, design doc, or feature proposal. Covers Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "create a PRD for X". Reserve this for changes where getting the design wrong is expensive to unwind — new cross-cutting mechanisms, schema changes, anything touching multiple subsystems. A one-file bug fix doesn't need this process. +**A UI feature that needs to survive a page reload, coordinate across tabs, or react to a backend push/poll is a cross-cutting mechanism even when it looks like "just a small UI feature."** It's easy to start implementing straight away because the visible surface (a badge, an icon) looks trivial - but the hard part is always the state-propagation mechanism underneath, and that mechanism usually has hidden dependencies on other code that already touches the same data. A real case: a "settings still applying" indicator went through three full implementation-and-break cycles (a cookie with a guessed timeout, a new PHP endpoint duplicating existing logic, a `localStorage`/polling redesign with its own bugs) before anyone traced every existing consumer of `app_state.json` and found that `front/js/sse_manager.js`'s `handleStateUpdate()` already pushed the exact signal needed to every open page. Step 4 ("trace every downstream consumer") below would have caught this on attempt #1 if it had been applied before writing any code, not after three failures. + ## Core principle: a PRD is a claim-verification exercise, not a writing exercise Every sentence that asserts something about how the code currently works must be checked against the actual code before it goes in — not written from memory, not inferred from a plugin's name or reputation, not assumed because it sounds plausible. Two failure patterns to watch for: diff --git a/.gemini/skills/pr-analysis/SKILL.md b/.gemini/skills/pr-analysis/SKILL.md index edd32ba48..8202014db 100644 --- a/.gemini/skills/pr-analysis/SKILL.md +++ b/.gemini/skills/pr-analysis/SKILL.md @@ -47,7 +47,7 @@ For each comment, determine: 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. +3. **Prepare a plan** — list each file and the exact change required. If a comment calls for new logic (a new check, helper, or condition), search for an existing equivalent first - the whole file being edited, not just the section in question, plus sibling pages/the Python backend - and extract/reuse it rather than planning a parallel implementation (see `code-standards`' DRY Principle section). 4. **Make changes one comment at a time** — keep commits focused. For a comment claiming a bug: write the test that should catch it first, run it against the current (unfixed) code, and confirm it fails for the right reason before writing the fix - see `prd-writing`'s test-first step for why (a test that never failed red can't be trusted to have caught anything). 5. **Rerun tests after each change** (`testing-workflow` skill) - confirm the new/updated test now passes, not just that nothing else broke. 6. **Reply** only after the commit is pushed. Include the short SHA. diff --git a/.gemini/skills/prd-writing/SKILL.md b/.gemini/skills/prd-writing/SKILL.md index 41fdb1896..313d7b26f 100644 --- a/.gemini/skills/prd-writing/SKILL.md +++ b/.gemini/skills/prd-writing/SKILL.md @@ -9,6 +9,8 @@ description: Rigorous PRD-writing methodology — challenge the idea, verify eve Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "create a PRD for X". Reserve this for changes where getting the design wrong is expensive to unwind — new cross-cutting mechanisms, schema changes, anything touching multiple subsystems. A one-file bug fix doesn't need this process. +**A UI feature that needs to survive a page reload, coordinate across tabs, or react to a backend push/poll is a cross-cutting mechanism even when it looks like "just a small UI feature."** It's easy to start implementing straight away because the visible surface (a badge, an icon) looks trivial - but the hard part is always the state-propagation mechanism underneath, and that mechanism usually has hidden dependencies on other code that already touches the same data. A real case: a "settings still applying" indicator went through three full implementation-and-break cycles (a cookie with a guessed timeout, a new PHP endpoint duplicating existing logic, a `localStorage`/polling redesign with its own bugs) before anyone traced every existing consumer of `app_state.json` and found that `front/js/sse_manager.js`'s `handleStateUpdate()` already pushed the exact signal needed to every open page. Step 4 ("trace every downstream consumer") below would have caught this on attempt #1 if it had been applied before writing any code, not after three failures. + ## Core principle: a PRD is a claim-verification exercise, not a writing exercise Every sentence that asserts something about how the code currently works must be checked against the actual code before it goes in — not written from memory, not inferred from a plugin's name or reputation, not assumed because it sounds plausible. Two failure patterns to watch for: diff --git a/.github/skills/code-standards/SKILL.md b/.github/skills/code-standards/SKILL.md index 41a1a90ba..07c7c3066 100644 --- a/.github/skills/code-standards/SKILL.md +++ b/.github/skills/code-standards/SKILL.md @@ -27,6 +27,7 @@ description: NetAlertX coding standards and conventions. Use this when writing c - when using `server/logger.py` `mylog()`, only use valid levels: `none`, `minimal`, `verbose`, `debug`, `trace`; invalid levels silently degrade to `none` - every Python function/method needs a succinct docstring describing its current use and behavior — not what changed or why (see Docstrings section below) - before adding a new frontend language string, search `front/php/templates/language/en_us.json` for an existing key with the same text/purpose and reuse it — don't add a near-duplicate key just because it's needed on a new page (see Language Strings section below) +- never add new server-side PHP logic (a new endpoint, new computation inside an existing PHP file) — `front/` is being migrated away from PHP, so any new backend state/computation belongs in the Python server, exposed to the frontend via an existing read path (see PHP/Python Boundary section below) ## File Length @@ -37,6 +38,10 @@ Keep code files under 500 lines. Split larger files into modules. Do not re-implement functionality. Reuse existing methods or refactor to create shared methods. +**This is a required pre-step, not a cleanup pass to do later.** Before writing any new check/condition/helper, search for an existing implementation of the same or similar logic first - grep the codebase, and read the *whole* file you're already touching, not just the section being edited. If something equivalent exists, extract it into a shared function and call it from the new site instead of writing a parallel implementation. + +A real case this was missed on: a new frontend indicator needed to know "is the backend still applying a settings change." That exact check already existed inline in `settings.php`'s own polling loop (`handleLoadingDialog()`, further down the same file being edited) - it took two rounds of reinventing it elsewhere (a cookie-based guess, then a duplicate PHP endpoint computing the same thing a second time) before it got extracted into one shared function (`isSettingsPending()` in `common.js`) that both the original page and the new consumer call. Read the existing code first; refactor into something reusable *while* implementing, not after a reviewer points out the duplication. + ## Database Access - Never access DB directly from application layers @@ -109,6 +114,26 @@ grep -n "Next\|Previous\|Showing" front/php/templates/language/en_us.json Prefer the generic `Gen_*` keys (e.g. `Gen_Prev`, `Gen_Next`) over a page-scoped name (`Presence_Page_Prev`) for genuinely generic UI text — a future page needing the same label should find it already there. Only add a new key when nothing existing fits; only that one file needs the addition — `getString()`/`lang()` fall back to the English string for any locale missing a key, so the other ~23 locale files don't need touching. +## PHP/Python Boundary — No New PHP Backend Logic + +`front/` is being migrated away from PHP. Never add a new PHP endpoint, or new server-side computation inside an existing PHP file - if a feature needs backend state or computation, it belongs in the Python server (`server/`), exposed to the frontend through an existing read path: + +- `app_state.json`, read via the generic `front/php/server/query_json.php` file-passthrough (no settings/state-specific logic lives in that file - it just serves raw JSON) +- `table_settings.json` (same passthrough) +- an existing REST or GraphQL endpoint + +A real case this was caught on: a new "settings still applying" UI indicator needed to know whether the backend had caught up on a config reload. The correct signal (`showSpinner` state + a config-file-mtime comparison) already existed in Python (`server/initialise.py`'s `importConfigs()`) - the first draft instead re-derived the same comparison in a new PHP endpoint, duplicating logic that the Python backend already computed and should have just exposed into existing shared state. + +Editing *existing* PHP page logic - templating, fixing a bug like a broken `explode()` parse, wiring up a new `
` - is fine and expected during the migration period. This rule is about not growing the PHP surface area with new backend-side logic, not about avoiding PHP entirely. + +## No Test Harness? Simulate Before Asking for a Live Test + +`front/` has no automated JS/PHP test suite. That makes it *more* important to verify a change before calling it done, not less - without a harness, "the user tests it live" becomes the only feedback loop, and that loop is slow and expensive (a real save, a real scan cycle, real timing) compared to a throwaway script. + +Before telling anyone a JS/PHP change is ready to test: write a small disposable Node (or PHP CLI) script that extracts the actual function(s) involved and runs them against realistic inputs - including the inputs that come from a different code path than the one being edited (a real `app_state.json` sample, a real cookie value, a renamed parameter actually being passed through). Do this on the *first* attempt, not after a live test comes back broken. + +A real case: a settings-reload indicator went through several rounds of "should work" before any of its logic was actually run. A standalone simulation run at that point would have immediately caught a renamed-parameter typo that a diff review missed, and an ordering bug (a cookie needing to clear before a reload fires, not inside the reload's own callback) - both found only after a live test failed, when a five-line script could have found them in seconds. + ## Devcontainer Constraints - Never `chmod` or `chown` during operations diff --git a/.github/skills/pr-analysis/SKILL.md b/.github/skills/pr-analysis/SKILL.md index 0a9c37da5..a9aaa2513 100644 --- a/.github/skills/pr-analysis/SKILL.md +++ b/.github/skills/pr-analysis/SKILL.md @@ -47,7 +47,7 @@ For each comment, determine: 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. +3. **Prepare a plan** — list each file and the exact change required. If a comment calls for new logic (a new check, helper, or condition), search for an existing equivalent first - the whole file being edited, not just the section in question, plus sibling pages/the Python backend - and extract/reuse it rather than planning a parallel implementation (see `code-standards`' DRY Principle section). 4. **Make changes one comment at a time** — keep commits focused. For a comment claiming a bug: write the test that should catch it first, run it against the current (unfixed) code, and confirm it fails for the right reason before writing the fix - see `prd-writing`'s test-first step for why (a test that never failed red can't be trusted to have caught anything). 5. **Rerun tests after each change** (`testing-workflow` skill) - confirm the new/updated test now passes, not just that nothing else broke. 6. **Reply** only after the commit is pushed via `report_progress`. Include the short SHA. diff --git a/.github/skills/prd-writing/SKILL.md b/.github/skills/prd-writing/SKILL.md index c0fbdbe76..f74c3cc15 100644 --- a/.github/skills/prd-writing/SKILL.md +++ b/.github/skills/prd-writing/SKILL.md @@ -9,6 +9,8 @@ description: Rigorous PRD-writing methodology for NetAlertX — challenge the id Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "create a PRD for X". Reserve this for changes where getting the design wrong is expensive to unwind — new cross-cutting mechanisms, schema changes, anything touching multiple subsystems. A one-file bug fix doesn't need this process. +**A UI feature that needs to survive a page reload, coordinate across tabs, or react to a backend push/poll is a cross-cutting mechanism even when it looks like "just a small UI feature."** It's easy to start implementing straight away because the visible surface (a badge, an icon) looks trivial - but the hard part is always the state-propagation mechanism underneath, and that mechanism usually has hidden dependencies on other code that already touches the same data. A real case: a "settings still applying" indicator went through three full implementation-and-break cycles (a cookie with a guessed timeout, a new PHP endpoint duplicating existing logic, a `localStorage`/polling redesign with its own bugs) before anyone traced every existing consumer of `app_state.json` and found that `front/js/sse_manager.js`'s `handleStateUpdate()` already pushed the exact signal needed to every open page. Step 4 ("trace every downstream consumer") below would have caught this on attempt #1 if it had been applied before writing any code, not after three failures. + ## Core principle: a PRD is a claim-verification exercise, not a writing exercise Every sentence that asserts something about how the code currently works must be checked against the actual code before it goes in — not written from memory, not inferred from a plugin's name or reputation, not assumed because it sounds plausible. Two failure patterns to watch for: diff --git a/CLAUDE.md b/CLAUDE.md index 87c774b29..e36b7ea20 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,3 +91,7 @@ Procedural/how-to knowledge (running tests, resetting the DB, devcontainer manag - 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). 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 real translation; other locales fall back to it automatically at runtime for a key they don't have. After adding or changing any key in `en_us.json`, run `python3 front/php/templates/language/merge_translations.py` (plain stdlib, no deps) - it re-sorts `en_us.json` alphabetically and propagates the new key into every other locale file with an empty placeholder value, so translators see what needs translating. Skipping this leaves the other 23 locale files out of sync with `en_us.json`'s key set. +- **Search before you build.** Before writing a new check/condition/helper for something (an "is X true" computation, a UI state signal, a utility), search the codebase for an existing implementation of the same or similar logic first - the same file (read the whole file, not just the section being edited), a sibling page, the Python backend. If one exists, extract it into a shared function and call it from the new site; don't write a parallel implementation planning to deduplicate later. A real case: a new frontend indicator needed to know "is the backend still applying a settings change" - that exact check already existed inline in `settings.php`'s own polling loop (`handleLoadingDialog()`), found only after two rounds of reinventing it elsewhere (a cookie-based guess, then a duplicate PHP endpoint) instead of reading the rest of the file first. +- **No new PHP backend logic.** `front/` is being migrated away from PHP, so never add a new PHP endpoint or new server-side computation inside an existing PHP file. If a feature needs backend state or computation, add it to the Python server and expose it to the frontend through an existing read path (`app_state.json` via `query_json.php`, `table_settings.json`, a REST/GraphQL endpoint) - never re-derive logic in PHP that the Python side already knows or could easily expose. Editing existing PHP page logic (templating, bug fixes) is fine; this is about not growing the PHP surface area. +- **A stateful UI feature (survives a reload, coordinates across tabs, reacts to a backend push) is a cross-cutting mechanism, not "just a UI feature."** Treat it like one before writing code: trace every existing consumer of the data it needs (e.g. everything that already reads `app_state.json`), not just the one file being edited. The visible surface looking small (a badge, an icon) says nothing about whether the state-propagation mechanism underneath already exists elsewhere. +- **No JS/PHP test harness exists in `front/` — simulate before asking for a live test, every time, not after a live test fails.** Write a disposable Node/PHP script that runs the actual function(s) against realistic inputs (including inputs crossing from a different file than the one being edited) before calling a change ready to test. A diff review misses things a five-second script run catches - a renamed parameter still referenced by its old name, an ordering assumption that's wrong once two async steps are both in play. diff --git a/front/css/app.css b/front/css/app.css index fdc2afbfc..739c47726 100755 --- a/front/css/app.css +++ b/front/css/app.css @@ -1604,6 +1604,26 @@ textarea[readonly], font-size: smaller; } +.main-header .sidebar-toggle +{ + /* .nav-pending-dot below needs a positioned ancestor to anchor to - + .sidebar-toggle has none by default (AdminLTE.css only sets float:left), + so without this it escapes to the nearest positioned element elsewhere + on the page instead of sitting on the toggle icon itself. */ + position: relative; +} + +.nav-pending-dot +{ + position: absolute; + top: 14px; + right: 10px; + width: 8px; + height: 8px; + border-radius: 50%; + display: inline-block; +} + .drag { cursor: move; /* fallback if grab cursor is unsupported */ diff --git a/front/js/common.js b/front/js/common.js index b6d763c7e..0e6fa49c1 100755 --- a/front/js/common.js +++ b/front/js/common.js @@ -902,6 +902,41 @@ function isRandomMAC(mac) // getDevDataByMac, cacheDevices, devicesListAll_JSON moved to cache.js +// ----------------------------------------------------------------------------- +/** + * Returns true if the backend hasn't yet confirmed importing settings as + * recent as referenceTimeMs (appState.settingsImported, from app_state.json). + * No fixed timeout: server/__main__.py's main loop only calls importConfigs() + * at the top of each iteration, and a full scan cycle (every plugin, + * potentially tens of thousands of objects) can legitimately take minutes, + * so this stays pending for exactly as long as the backend actually takes. + * Used by settings.php's own handleLoadingDialog(), passing the config + * file's mtime*1000 (via PHP's filemtime()) as referenceTimeMs - that page's + * own full-page blocking spinner, unrelated to the settingsPendingReload + * nav indicator (handle_pending_settings.js / sse_manager.js), which doesn't + * need a reference time at all since its resolution is pushed via SSE. + * @param {object} appState - parsed app_state.json. + * @param {number} referenceTimeMs - a moment (ms since epoch) that should + * already be reflected in settingsImported if the backend has caught up. + * @returns {boolean} + */ +function isSettingsPending(appState, referenceTimeMs) { + var importedMs = parseInt(appState["settingsImported"] * 1000, 10); + return referenceTimeMs > importedMs; +} + +// ----------------------------------------------------------------------------- +/** + * Shows/hides the sidebar-toggle's attention dot based on whether any + * .info-icon-nav badge in the sidebar is currently visible (not .myhidden) - + * deliberately doesn't know which badge triggered it, so a future badge + * lights this dot up for free without this function needing to change. + */ +function updateNavPendingDot() { + var anyVisible = $('.info-icon-nav').not('.myhidden').length > 0; + $('#navPendingDot').toggleClass('myhidden', !anyVisible); +} + // ----------------------------------------------------------------------------- function isEmpty(value) { diff --git a/front/js/handle_pending_settings.js b/front/js/handle_pending_settings.js new file mode 100644 index 000000000..4ff6d4d8f --- /dev/null +++ b/front/js/handle_pending_settings.js @@ -0,0 +1,14 @@ +//-------------------------------------------------------------- +// Show the "settings still applying" indicator on page load if a save left +// the settingsPendingReload cookie set (front/settings.php's save handler). +// No polling: resolution is pushed via SSE and handled entirely in +// sse_manager.js's handleStateUpdate() (step 4), which clears this same +// cookie the moment appState.settingsImported confirms the import landed. +function settingsPendingUpdateUI() { + var isPending = getCookie("settingsPendingReload") === "true"; + + $('#settingsPendingReload').toggleClass('myhidden', !isPending); + updateNavPendingDot(); +} + +settingsPendingUpdateUI(); diff --git a/front/js/handle_version.js b/front/js/handle_version.js index 01b3eb607..24bda4afa 100755 --- a/front/js/handle_version.js +++ b/front/js/handle_version.js @@ -21,13 +21,15 @@ function versionUpdateUI(){ maintenanceDiv = $('#current-version-text') } - // handling the maintenance section message + // handling the maintenance section message if(emptyArr.includes(maintenanceDiv) == false && $(maintenanceDiv).length != 0) - { + { $(maintenanceDiv).attr("class", $(maintenanceDiv).attr("class").replace("myhidden", "")) - } + } -} + updateNavPendingDot(); + +} //-------------------------------------------------------------- // Checks if a new version is available via the global app_state.json diff --git a/front/js/sse_manager.js b/front/js/sse_manager.js index c8536a2a5..211e8c6a2 100644 --- a/front/js/sse_manager.js +++ b/front/js/sse_manager.js @@ -170,6 +170,14 @@ class NetAlertXStateManager { const importedMs = parseInt(appState["settingsImported"] * 1000); const lastReloaded = parseInt(getCache(CACHE_KEYS.INIT_TIMESTAMP)); if (importedMs > lastReloaded) { + // Clear the settings-pending indicator (cookie + DOM) synchronously, + // before scheduling the reload below - not inside clearCache()'s own + // timeout. Otherwise the freshly-reloaded page would briefly re-read + // the still-present cookie and flash the indicator back on. + setCookie("settingsPendingReload", "", -1); + $('#settingsPendingReload').addClass('myhidden'); + updateNavPendingDot(); + console.log("[NetAlertX State] Settings changed — clearing cache and reloading"); setTimeout(() => clearCache(), 500); } diff --git a/front/php/templates/footer.php b/front/php/templates/footer.php index dc6998772..3161cdea9 100755 --- a/front/php/templates/footer.php +++ b/front/php/templates/footer.php @@ -56,7 +56,8 @@ - + + diff --git a/front/php/templates/header.php b/front/php/templates/header.php index 481764115..8d85a5b65 100755 --- a/front/php/templates/header.php +++ b/front/php/templates/header.php @@ -185,6 +185,10 @@ + + @@ -402,6 +406,10 @@
  • + +
    + +
    diff --git a/front/php/templates/language/ar_ar.json b/front/php/templates/language/ar_ar.json index 2b63a2e82..706bde08f 100644 --- a/front/php/templates/language/ar_ar.json +++ b/front/php/templates/language/ar_ar.json @@ -817,6 +817,7 @@ "general_event_title": "عنوان الحدث العام", "go_to_device_event_tooltip": "انتقل إلى الجهاز", "go_to_node_event_tooltip": "تلميح الانتقال إلى العقدة", + "nav_pending_dot": "", "new_version_available": "يتوفر إصدار جديد", "report_guid": "معرف التقرير", "report_guid_missing": "معرف التقرير مفقود", @@ -841,6 +842,7 @@ "settings_other_scanners": "إضافات الماسح الضوئي الأخرى غير الخاصة بالأجهزة والتي يتم تفعيلها حاليًا.", "settings_other_scanners_icon": "fa-صلب fa-إعادة تدوير", "settings_other_scanners_label": "الماسحات الضوئية الأخرى", + "settings_pending_reload": "", "settings_publishers": "تم تفعيل بوابات الإشعارات - الناشرين، الذين سيرسلون إشعارًا بناءً على إعداداتك.", "settings_publishers_icon": "أيقونة الناشرين", "settings_publishers_info": "قم بتحميل المزيد من الناشرين باستخدام إعداد     LOADED_PLUGINS", diff --git a/front/php/templates/language/ca_ca.json b/front/php/templates/language/ca_ca.json index df2df995b..8a65378ca 100644 --- a/front/php/templates/language/ca_ca.json +++ b/front/php/templates/language/ca_ca.json @@ -817,6 +817,7 @@ "general_event_title": "Execució d'un esdeveniment ad-hoc", "go_to_device_event_tooltip": "Navegar al dispositiu", "go_to_node_event_tooltip": "Navegació a la pàgina de la Xarxa del node donat", + "nav_pending_dot": "", "new_version_available": "Ja està disponible una nova versió.", "report_guid": "Notificació guid:", "report_guid_missing": "No s'ha trobat la notificació enllaçada. Hi ha un petit retard entre les notificacions enviades recentment i que estiguin disponibles. Refresqui la pàgina i la memòria cau d'aquí uns segons. També és possible que la notificació seleccionada s'hagi esborrat durant el manteniment tal com s'especifica a la configuració DBCLNP_NOTIFI_HIST.

    L'última notificació es mostra en el seu lloc. La notificació perduda té el següent GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Uns altres plugins no relacionats amb dispositius que estan actualment activats.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Altres escàners", + "settings_pending_reload": "", "settings_publishers": "Altres passarel·les de notificació i edició que enviaran una notificació en funció de la configuració.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Carregar més Editors amb la configuració LOADED_PLUGINS", @@ -852,4 +854,4 @@ "settings_system_label": "Sistema", "settings_update_item_warning": "Actualitza el valor sota. Sigues curós de seguir el format anterior. No hi ha validació.", "test_event_tooltip": "Deseu els canvis primer abans de comprovar la configuració." -} +} \ No newline at end of file diff --git a/front/php/templates/language/cs_cz.json b/front/php/templates/language/cs_cz.json index 8fd37258d..a71d7067d 100644 --- a/front/php/templates/language/cs_cz.json +++ b/front/php/templates/language/cs_cz.json @@ -817,6 +817,7 @@ "general_event_title": "Vykonávání jednorázové události", "go_to_device_event_tooltip": "Přejít na zařízení", "go_to_node_event_tooltip": "Přejít na stránku Síť daného uzlu", + "nav_pending_dot": "", "new_version_available": "Je k dispozici nová verze.", "report_guid": "Guid notifikace:", "report_guid_missing": "Odkazovaná notifikace nenalezena. Je zde drobná prodleva mezi právě zaslanými notifikacemi a jejich viditelností. Znovunačtěte stránka za několik sekund. Je také možné, že vybraná notifikace byla mezitím smazána při údržbě, jak je specifikováno v nastavení DBCLNP_NOTIFI_HIST.

    Namísto toho je zobrazena nejnovější notifikace. Chybějící notifikace má následující GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Ostatní v tuto chvíli zapnuté zásuvné moduly, které nejsou skenery zařízení.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Ostatní skenery", + "settings_pending_reload": "", "settings_publishers": "Zapnuté brány notifikací – vydavatelé, kteří odešlou notifikaci na základě vašich nastavení.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Načíst další Vydavatele pomocí nastavení LOADED_PLUGINS", diff --git a/front/php/templates/language/de_de.json b/front/php/templates/language/de_de.json index 37d29bfd4..488a64178 100644 --- a/front/php/templates/language/de_de.json +++ b/front/php/templates/language/de_de.json @@ -890,6 +890,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "Zum Gerät navigieren", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "Es ist eine neue Version verfügbar.", "report_guid": "", "report_guid_missing": "", @@ -914,6 +915,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "Andere Scanner", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "Lade mehr Veröffentlicher mit den geladene Plugins-Einstellungen", @@ -925,4 +927,4 @@ "settings_system_label": "System", "settings_update_item_warning": "", "test_event_tooltip": "Speichere die Änderungen, bevor Sie die Einstellungen testen." -} +} \ No newline at end of file diff --git a/front/php/templates/language/en_us.json b/front/php/templates/language/en_us.json index 93d1e7422..095fc7b75 100755 --- a/front/php/templates/language/en_us.json +++ b/front/php/templates/language/en_us.json @@ -817,6 +817,7 @@ "general_event_title": "Executing an ad-hoc event", "go_to_device_event_tooltip": "Navigate to the device", "go_to_node_event_tooltip": "Navigate to the Network page of the given node", + "nav_pending_dot": "There are some in-menu notifications shown.", "new_version_available": "A new version is available.", "report_guid": "Notification guid:", "report_guid_missing": "Linked notification not found. There is a small delay between recently sent notifications and them being available. Referesh your page and cache after a few seconds. It's also possible the selected notification have been deleted during maintenance as specified in the DBCLNP_NOTIFI_HIST setting.

    The latest notification is displayed instead. The missing notification has the following GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Other, non-device scanner plugins that are currently enabled.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Other scanners", + "settings_pending_reload": "Settings saved - the backend is still applying them, this usually takes some time depending on background scans.", "settings_publishers": "Enabled notification gateways - publishers, that will send a notification depending on your settings.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Load more Publishers with the LOADED_PLUGINS setting", diff --git a/front/php/templates/language/es_es.json b/front/php/templates/language/es_es.json index 402688567..00b84fbe8 100644 --- a/front/php/templates/language/es_es.json +++ b/front/php/templates/language/es_es.json @@ -888,6 +888,7 @@ "general_event_title": "Ejecutar un evento ad-hoc", "go_to_device_event_tooltip": "Navegar al dispositivo", "go_to_node_event_tooltip": "Vaya a la página de Red del nodo indicado", + "nav_pending_dot": "", "new_version_available": "Una nueva versión está disponible.", "report_guid": "Guía de las notificaciones:", "report_guid_missing": "No se ha encontrado la notificación vinculada. Hay un pequeño retraso entre las notificaciones enviadas recientemente y su disponibilidad. Actualiza tu página y la caché después de unos segundos. También es posible que la notificación seleccionada se haya eliminado durante el mantenimiento, tal y como se especifica en la configuración de DBCLNP_NOTIFI_HIST.

    En su lugar, se muestra la notificación más reciente. La notificación que falta tiene el siguiente GUID:", @@ -912,6 +913,7 @@ "settings_other_scanners": "Otros plugins de escáner no relacionados con dispositivos que están activados actualmente.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Otros escáneres", + "settings_pending_reload": "", "settings_publishers": "Puertas de enlace para las notificación habilitadas: editores, que enviarán una notificación según su configuración.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Cargue más editor@s con el ajuste LOADED_PLUGINS", @@ -923,4 +925,4 @@ "settings_system_label": "Sistema", "settings_update_item_warning": "Actualice el valor a continuación. Tenga cuidado de seguir el formato anterior. O la validación no se realiza.", "test_event_tooltip": "Guarda tus cambios antes de probar nuevos ajustes." -} +} \ No newline at end of file diff --git a/front/php/templates/language/fa_fa.json b/front/php/templates/language/fa_fa.json index c12073226..d73ed0a35 100644 --- a/front/php/templates/language/fa_fa.json +++ b/front/php/templates/language/fa_fa.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/fi_fi.json b/front/php/templates/language/fi_fi.json index e27e3e250..3a1263454 100644 --- a/front/php/templates/language/fi_fi.json +++ b/front/php/templates/language/fi_fi.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/fr_fr.json b/front/php/templates/language/fr_fr.json index c48a13620..5e356eec2 100644 --- a/front/php/templates/language/fr_fr.json +++ b/front/php/templates/language/fr_fr.json @@ -817,6 +817,7 @@ "general_event_title": "Lancement d'un événement sur mesure", "go_to_device_event_tooltip": "Naviguer vers cet appareil", "go_to_node_event_tooltip": "Aller vers la page Réseau du nœud concerné", + "nav_pending_dot": "", "new_version_available": "Une nouvelle version est disponible.", "report_guid": "GUID de la notification :", "report_guid_missing": "La notification associée n'a pas été trouvée. Un petit délai existe entre l'envoi d'une notification et sa disponibilité réelle pour affichage. Rafraichissez la page et votre cache après quelques secondes. Il est aussi possible que la notification sélectionnée ait été supprimée durant une opération de maintenance, comme renseigné dans le paramètre DBCLNP_NOTIFI_HIST.

    La dernière notification est affichée à sa place. La notification manquante dispose du GUID suivant :", @@ -841,6 +842,7 @@ "settings_other_scanners": "Autres plugins activés, hors scanners d'appareils.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Autres scanners", + "settings_pending_reload": "", "settings_publishers": "Activer les passerelles de publication de notifications, qui enverront une notification en fonction de vos paramètres renseignés.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Charger plus de passerelles de publication avec le paramètre LOADED_PLUGINS", @@ -852,4 +854,4 @@ "settings_system_label": "Système", "settings_update_item_warning": "Mettre à jour la valeur ci-dessous. Veillez à bien suivre le même format qu'auparavant. Il n'y a pas de pas de contrôle.", "test_event_tooltip": "Enregistrer d'abord vos modifications avant de tester vôtre paramétrage." -} +} \ No newline at end of file diff --git a/front/php/templates/language/he_il.json b/front/php/templates/language/he_il.json index e27e3e250..3a1263454 100644 --- a/front/php/templates/language/he_il.json +++ b/front/php/templates/language/he_il.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/hu_hu.json b/front/php/templates/language/hu_hu.json index e27e3e250..3a1263454 100644 --- a/front/php/templates/language/hu_hu.json +++ b/front/php/templates/language/hu_hu.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/id_id.json b/front/php/templates/language/id_id.json index e27e3e250..3a1263454 100644 --- a/front/php/templates/language/id_id.json +++ b/front/php/templates/language/id_id.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/it_it.json b/front/php/templates/language/it_it.json index 45d0cabb1..50694bae0 100644 --- a/front/php/templates/language/it_it.json +++ b/front/php/templates/language/it_it.json @@ -817,6 +817,7 @@ "general_event_title": "Esecuzione di un evento ad-hoc", "go_to_device_event_tooltip": "Naviga al dispositivo", "go_to_node_event_tooltip": "Passa alla pagina Rete del nodo specificato", + "nav_pending_dot": "", "new_version_available": "È disponibile una nuova versione.", "report_guid": "GUID notifica:", "report_guid_missing": "Notifica collegata non trovata. C'è un piccolo ritardo tra la disponibilità delle notifiche inviate di recente e la loro disponibilità. Aggiorna la pagina e la cache dopo alcuni secondi. È anche possibile che la notifica selezionata sia stata eliminata durante la manutenzione come specificato nell'impostazione DBCLNP_NOTIFI_HIST.

    Viene invece visualizzata l'ultima notifica. La notifica mancante ha il seguente GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Altri plugin, non scanner per dispositivi, che sono attualmente abilitati.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Altri scanner", + "settings_pending_reload": "", "settings_publishers": "Gateway/editori di notifica abilitati, che invieranno una notifica in base alle tue impostazioni.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Carica più editori con l'impostazione LOADED_PLUGINS", @@ -852,4 +854,4 @@ "settings_system_label": "Sistema", "settings_update_item_warning": "Aggiorna il valore qui sotto. Fai attenzione a seguire il formato precedente. La convalida non viene eseguita.", "test_event_tooltip": "Salva le modifiche prima di provare le nuove impostazioni." -} +} \ No newline at end of file diff --git a/front/php/templates/language/ja_jp.json b/front/php/templates/language/ja_jp.json index bc61e88d0..fca4b18e6 100644 --- a/front/php/templates/language/ja_jp.json +++ b/front/php/templates/language/ja_jp.json @@ -817,6 +817,7 @@ "general_event_title": "アドホックイベントの実行", "go_to_device_event_tooltip": "デバイスに移動", "go_to_node_event_tooltip": "指定されたノードのネットワークページに移動する", + "nav_pending_dot": "", "new_version_available": "新しいバージョンが利用可能です。", "report_guid": "通知guid:", "report_guid_missing": "リンクされた通知が見つかりません。送信された通知が利用可能になるまで、わずかな遅延が生じます。数秒後にページとキャッシュを更新してください。また、DBCLNP_NOTIFI_HIST 設定で指定されているメンテナンス中に、選択した通知が削除された可能性もあります。

    代わりに最新の通知が表示されます。欠落している通知のGUIDは以下の通りです:", @@ -841,6 +842,7 @@ "settings_other_scanners": "現在有効になっている、デバイス以外のスキャナープラグイン。", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "その他のスキャナー", + "settings_pending_reload": "", "settings_publishers": "有効化された通知ゲートウェイ - 設定に応じて通知を送信する発行元。", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "LOADED_PLUGINS 設定でさらに多くのパブリッシャーを読み込みます", diff --git a/front/php/templates/language/nb_no.json b/front/php/templates/language/nb_no.json index 5f951ca99..e7736b043 100644 --- a/front/php/templates/language/nb_no.json +++ b/front/php/templates/language/nb_no.json @@ -817,6 +817,7 @@ "general_event_title": "Utfører en ad-hoc hendelse", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "Notifikasjons GUID:", "report_guid_missing": "Koblet notifikasjon ikke funnet. Det er en liten forsinkelse mellom nylig sendt notifikasjoner og at de er tilgjengelige. Oppdater siden din og hurtigbufferen etter noen sekunder. Det er også mulig den valgte notifikasjonen er slettet under vedlikehold som spesifisert i DBCLNP_NOTIFI_HIST innstillingen.

    Den siste notifikasjonen vises i stedet. Den manglende notifikasjonen har følgende GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Andre ikke enheter-plugins som er aktivert.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Andre skannere", + "settings_pending_reload": "", "settings_publishers": "Aktivert notifikasjons-gateways - utgivere, som vil sende en notifikasjon avhengig av innstillingene dine.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "", diff --git a/front/php/templates/language/pl_pl.json b/front/php/templates/language/pl_pl.json index 06e966ecc..5bb9ff960 100644 --- a/front/php/templates/language/pl_pl.json +++ b/front/php/templates/language/pl_pl.json @@ -817,6 +817,7 @@ "general_event_title": "Wykonywanie zdarzenia ad-hoc", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "Przejdź do strony Sieć danego węzła", + "nav_pending_dot": "", "new_version_available": "Dostępna jest nowa wersja.", "report_guid": "GUID powiadomienia:", "report_guid_missing": "Nie znaleziono powiązanego powiadomienia. Istnieje niewielkie opóźnienie między momentem wysłania powiadomienia a jego dostępnością. Odśwież stronę i pamięć podręczną po kilku sekundach. Możliwe również, że wybrane powiadomienie zostało usunięte podczas konserwacji, zgodnie z ustawieniem DBCLNP_NOTIFI_HIST.

    Zamiast tego wyświetlane jest najnowsze powiadomienie. Brakujące powiadomienie ma następujący identyfikator GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Inne, niebędące skanerami urządzeń wtyczki, które są obecnie włączone.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Inne skanery", + "settings_pending_reload": "", "settings_publishers": "Włączone bramki powiadomień – publikatory, które będą wysyłać powiadomienia w zależności od twoich ustawień.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Załaduj więcej publikatorów za pomocą ustawienia LOADED_PLUGINS", diff --git a/front/php/templates/language/pt_br.json b/front/php/templates/language/pt_br.json index 307d35592..aa337b138 100644 --- a/front/php/templates/language/pt_br.json +++ b/front/php/templates/language/pt_br.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/pt_pt.json b/front/php/templates/language/pt_pt.json index e4e55b06b..86bfc5292 100644 --- a/front/php/templates/language/pt_pt.json +++ b/front/php/templates/language/pt_pt.json @@ -817,6 +817,7 @@ "general_event_title": "A executar um evento ad-hoc", "go_to_device_event_tooltip": "Navegar para o dispositivo", "go_to_node_event_tooltip": "Navegar para a página de Rede do nó em questão", + "nav_pending_dot": "", "new_version_available": "Uma versão nova está disponível.", "report_guid": "Guid de Notificação:", "report_guid_missing": "Notificação associada não foi encontrada. Há um pequeno atraso entre notificações recentemente enviadas e as mesmas estarem disponíveis. Atualize a sua página e cache após alguns segundos. Também é possível que a notificação selecionada tenha sido eliminada durante a manutenção como especificado na definição DBCLNP_NOTIFI_HIST.

    Em vez disso, a última notificação é mostrada. A notificação em falta tem o seguinte GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Outros plugins de scaneadores que não são do dispositivo estão atualmente ativos.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Outros scaneadores", + "settings_pending_reload": "", "settings_publishers": "Gateways de notificação ativados - editores que enviarão uma notificação de acordo com as suas definições.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Carregar mais Editores com a definição LOADED_PLUGINS", diff --git a/front/php/templates/language/ru_ru.json b/front/php/templates/language/ru_ru.json index 84c199428..edda373df 100644 --- a/front/php/templates/language/ru_ru.json +++ b/front/php/templates/language/ru_ru.json @@ -817,6 +817,7 @@ "general_event_title": "Выполнение специального события", "go_to_device_event_tooltip": "Перейти к устройству", "go_to_node_event_tooltip": "Переход на страницу \"Сеть\" данного узла", + "nav_pending_dot": "", "new_version_available": "Доступна новая версия.", "report_guid": "Идентификатор уведомления:", "report_guid_missing": "Связанное уведомление не найдено. Между недавно отправленными уведомлениями и их доступностью существует небольшая задержка. Обновите страницу и кэшируйте ее через несколько секунд. Также возможно, что выбранное уведомление было удалено во время обслуживания, как указано в настройке DBCLNP_NOTIFI_HIST.

    Вместо этого отображается последнее уведомление. Отсутствующее уведомление имеет следующий GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Другие плагины сканера, не относящиеся к устройствам, которые в настоящее время включены.", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "Другие сканеры", + "settings_pending_reload": "", "settings_publishers": "Включенные шлюзы уведомлений - сервисы, которые будут отправлять уведомления в зависимости от ваших настроек.", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "Загрузите больше нотификаторов с помощью настройки LOADED_PLUGINS", diff --git a/front/php/templates/language/sv_sv.json b/front/php/templates/language/sv_sv.json index e27e3e250..3a1263454 100644 --- a/front/php/templates/language/sv_sv.json +++ b/front/php/templates/language/sv_sv.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/tr_tr.json b/front/php/templates/language/tr_tr.json index 5ac870753..a48e6dc6c 100644 --- a/front/php/templates/language/tr_tr.json +++ b/front/php/templates/language/tr_tr.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "Diğer tarayıcılar", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/uk_ua.json b/front/php/templates/language/uk_ua.json index ec10abf90..639d7f425 100644 --- a/front/php/templates/language/uk_ua.json +++ b/front/php/templates/language/uk_ua.json @@ -817,6 +817,7 @@ "general_event_title": "Виконання спеціальної події", "go_to_device_event_tooltip": "Перейдіть до пристрою", "go_to_node_event_tooltip": "Перейдіть на сторінку Мережа даного вузла", + "nav_pending_dot": "", "new_version_available": "Доступна нова версія.", "report_guid": "Довідник сповіщень:", "report_guid_missing": "Пов’язане сповіщення не знайдено. Існує невелика затримка між нещодавно надісланими сповіщеннями та їх доступністю. Оновіть сторінку та кеш через кілька секунд. Також можливо, вибране сповіщення було видалено під час обслуговування, як зазначено в параметрі DBCLNP_NOTIFI_HIST.

    Натомість відображається останнє сповіщення. Відсутнє сповіщення має такий GUID:", @@ -841,6 +842,7 @@ "settings_other_scanners": "Інші наразі ввімкнені плагіни сканера, не пов’язані з пристроєм.", "settings_other_scanners_icon": "fa -solid fa-recycle", "settings_other_scanners_label": "Інші сканери", + "settings_pending_reload": "", "settings_publishers": "Увімкнені шлюзи сповіщень – видавці, які надсилатимуть сповіщення залежно від ваших налаштувань.", "settings_publishers_icon": "fa-твердий fa-паперовий літак", "settings_publishers_info": "Завантажте більше видавців за допомогою параметра LOADED_PLUGINS", diff --git a/front/php/templates/language/vi_vn.json b/front/php/templates/language/vi_vn.json index e27e3e250..3a1263454 100644 --- a/front/php/templates/language/vi_vn.json +++ b/front/php/templates/language/vi_vn.json @@ -817,6 +817,7 @@ "general_event_title": "", "go_to_device_event_tooltip": "", "go_to_node_event_tooltip": "", + "nav_pending_dot": "", "new_version_available": "", "report_guid": "", "report_guid_missing": "", @@ -841,6 +842,7 @@ "settings_other_scanners": "", "settings_other_scanners_icon": "", "settings_other_scanners_label": "", + "settings_pending_reload": "", "settings_publishers": "", "settings_publishers_icon": "", "settings_publishers_info": "", diff --git a/front/php/templates/language/zh_cn.json b/front/php/templates/language/zh_cn.json index 174f4521a..27c885ff6 100644 --- a/front/php/templates/language/zh_cn.json +++ b/front/php/templates/language/zh_cn.json @@ -817,6 +817,7 @@ "general_event_title": "执行自组织网络事件", "go_to_device_event_tooltip": "前往设备页面", "go_to_node_event_tooltip": "前往该节点的网络配置页", + "nav_pending_dot": "", "new_version_available": "新版本已发布。", "report_guid": "通知guid：", "report_guid_missing": "未找到链接的通知。最近发送的通知与可用通知之间存在短暂延迟。几秒钟后刷新页面并缓存。所选通知也可能已在维护期间被删除，如 DBCLNP_NOTIFI_HIST 设置中所述。

    系统将改为显示最新通知。缺失的通知具有以下 GUID：", @@ -841,6 +842,7 @@ "settings_other_scanners": "其他当前已启用的非设备扫描仪插件。", "settings_other_scanners_icon": "fa-solid fa-recycle", "settings_other_scanners_label": "其他扫描仪", + "settings_pending_reload": "", "settings_publishers": "启用通知网关 - 发布者，将根据您的设置发送通知。", "settings_publishers_icon": "fa-solid fa-paper-plane", "settings_publishers_info": "使用 LOADED_PLUGINS 设置加载更多发布商", diff --git a/front/settings.php b/front/settings.php index bd4ee4f80..cd99c0a7c 100755 --- a/front/settings.php +++ b/front/settings.php @@ -644,6 +644,20 @@ $settingsJSON_DB = json_encode($settings, JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX write_notification(`[Settings] Settings saved by the user`, 'info') + // Show the pending indicator immediately (optimistic - a save + // just succeeded, so "pending" is correct by definition) and + // persist it across navigation/reload as a plain boolean + // cookie, not localStorage (clearCache() below clears + // localStorage, which would wipe it immediately). Resolution + // is handled entirely by sse_manager.js's existing + // settingsImported-vs-INIT_TIMESTAMP check (step 4 of + // handleStateUpdate()), which clears this same cookie the + // moment the backend's SSE push confirms the import landed - + // nothing here polls or compares timestamps. + setCookie("settingsPendingReload", "true", 60); + $('#settingsPendingReload').removeClass('myhidden'); + updateNavPendingDot(); + if (requiresReloadWait) { clearCache() } else { @@ -699,7 +713,7 @@ $settingsJSON_DB = json_encode($settings, JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX // check if displayed settings are outdated - if(appState["showSpinner"] || fileModificationTime > importedMiliseconds) + if(isSettingsPending(appState, fileModificationTime)) { showSpinner("settings_old") showSettingsSkeleton() From 145f3778d089452b9b661ba31a40473f7acb5fbe Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 3 Oct 2026 13:23:55 +1000 Subject: [PATCH 12/13] FE+BE: save icon + columns SOT refactor --- CLAUDE.md | 1 + front/php/templates/header.php | 2 +- server/const.py | 44 +---- server/db/device_filter_columns.py | 64 ++++++++ .../db/sync_device_filter_columns_config.py | 52 ++++++ test/db/test_device_filter_columns.py | 153 ++++++++++++++++++ 6 files changed, 273 insertions(+), 43 deletions(-) create mode 100644 server/db/device_filter_columns.py create mode 100644 server/db/sync_device_filter_columns_config.py create mode 100644 test/db/test_device_filter_columns.py diff --git a/CLAUDE.md b/CLAUDE.md index e36b7ea20..c0061bb8e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,6 +91,7 @@ Procedural/how-to knowledge (running tests, resetting the DB, devcontainer manag - 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). 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 real translation; other locales fall back to it automatically at runtime for a key they don't have. After adding or changing any key in `en_us.json`, run `python3 front/php/templates/language/merge_translations.py` (plain stdlib, no deps) - it re-sorts `en_us.json` alphabetically and propagates the new key into every other locale file with an empty placeholder value, so translators see what needs translating. Skipping this leaves the other 23 locale files out of sync with `en_us.json`'s key set. +- A filterable Devices-table column is added in one place: `DEVICE_FILTER_COLUMNS` (`server/db/device_filter_columns.py`), which generates both `sql_devices_filters` (`server/const.py`) and (after running `python3 server/db/sync_device_filter_columns_config.py`) `server/plugins/ui_settings/config.json`'s `columns_filters.options[]`. Never hand-edit either generated side directly - `test/db/test_device_filter_columns.py`'s drift guard fails if the registry and `config.json` disagree. This is for the *filterable*-columns list only; the separate *displayable*-columns list (`device_columns.options[]`, `front/js/device-columns.js`) is untouched by this mechanism. - **Search before you build.** Before writing a new check/condition/helper for something (an "is X true" computation, a UI state signal, a utility), search the codebase for an existing implementation of the same or similar logic first - the same file (read the whole file, not just the section being edited), a sibling page, the Python backend. If one exists, extract it into a shared function and call it from the new site; don't write a parallel implementation planning to deduplicate later. A real case: a new frontend indicator needed to know "is the backend still applying a settings change" - that exact check already existed inline in `settings.php`'s own polling loop (`handleLoadingDialog()`), found only after two rounds of reinventing it elsewhere (a cookie-based guess, then a duplicate PHP endpoint) instead of reading the rest of the file first. - **No new PHP backend logic.** `front/` is being migrated away from PHP, so never add a new PHP endpoint or new server-side computation inside an existing PHP file. If a feature needs backend state or computation, add it to the Python server and expose it to the frontend through an existing read path (`app_state.json` via `query_json.php`, `table_settings.json`, a REST/GraphQL endpoint) - never re-derive logic in PHP that the Python side already knows or could easily expose. Editing existing PHP page logic (templating, bug fixes) is fine; this is about not growing the PHP surface area. - **A stateful UI feature (survives a reload, coordinates across tabs, reacts to a backend push) is a cross-cutting mechanism, not "just a UI feature."** Treat it like one before writing code: trace every existing consumer of the data it needs (e.g. everything that already reads `app_state.json`), not just the one file being edited. The visible surface looking small (a badge, an icon) says nothing about whether the state-propagation mechanism underneath already exists elsewhere. diff --git a/front/php/templates/header.php b/front/php/templates/header.php index 8d85a5b65..f6891c6b7 100755 --- a/front/php/templates/header.php +++ b/front/php/templates/header.php @@ -408,7 +408,7 @@
    - +
    diff --git a/server/const.py b/server/const.py index 067b194b0..54958592b 100755 --- a/server/const.py +++ b/server/const.py @@ -16,6 +16,7 @@ from config_paths import ( PLUGINS_PATH_WITH_TRAILING_SEP, REPORT_TEMPLATES_PATH_WITH_TRAILING_SEP, ) +from db.device_filter_columns import build_devices_filters_sql # =============================================================================== # PATHS @@ -69,48 +70,7 @@ sql_devices_all = """ """ sql_appevents = """select * from AppEvents order by dateTimeCreated desc""" -sql_devices_filters = f""" - SELECT DISTINCT 'devSite' AS columnName, devSite AS columnValue, devSite AS columnLabel - FROM Devices WHERE devSite NOT IN ({NULL_EQUIVALENTS_SQL}) AND devSite IS NOT NULL - UNION - SELECT DISTINCT 'devSourcePlugin' AS columnName, devSourcePlugin AS columnValue, devSourcePlugin AS columnLabel - FROM Devices WHERE devSourcePlugin NOT IN ({NULL_EQUIVALENTS_SQL}) AND devSourcePlugin IS NOT NULL - UNION - SELECT DISTINCT 'devOwner' AS columnName, devOwner AS columnValue, devOwner AS columnLabel - FROM Devices WHERE devOwner NOT IN ({NULL_EQUIVALENTS_SQL}) AND devOwner IS NOT NULL - UNION - SELECT DISTINCT 'devType' AS columnName, devType AS columnValue, devType AS columnLabel - FROM Devices WHERE devType NOT IN ({NULL_EQUIVALENTS_SQL}) AND devType IS NOT NULL - UNION - SELECT DISTINCT 'devGroup' AS columnName, devGroup AS columnValue, devGroup AS columnLabel - FROM Devices WHERE devGroup NOT IN ({NULL_EQUIVALENTS_SQL}) AND devGroup IS NOT NULL - UNION - SELECT DISTINCT 'devLocation' AS columnName, devLocation AS columnValue, devLocation AS columnLabel - FROM Devices WHERE devLocation NOT IN ({NULL_EQUIVALENTS_SQL}) AND devLocation IS NOT NULL - UNION - SELECT DISTINCT 'devVendor' AS columnName, devVendor AS columnValue, devVendor AS columnLabel - FROM Devices WHERE devVendor NOT IN ({NULL_EQUIVALENTS_SQL}) AND devVendor IS NOT NULL - UNION - SELECT DISTINCT 'devSyncHubNode' AS columnName, devSyncHubNode AS columnValue, devSyncHubNode AS columnLabel - FROM Devices WHERE devSyncHubNode NOT IN ({NULL_EQUIVALENTS_SQL}) AND devSyncHubNode IS NOT NULL - UNION - SELECT DISTINCT 'devVlan' AS columnName, devVlan AS columnValue, devVlan AS columnLabel - FROM Devices WHERE devVlan NOT IN ({NULL_EQUIVALENTS_SQL}) AND devVlan IS NOT NULL - UNION - SELECT 'devParentMAC' AS columnName, d.devParentMAC AS columnValue, - COALESCE(p.devName, d.devParentMAC) AS columnLabel - FROM Devices d - LEFT JOIN Devices p ON LOWER(p.devMac) = LOWER(d.devParentMAC) - WHERE d.devParentMAC NOT IN ({NULL_EQUIVALENTS_SQL}) AND d.devParentMAC IS NOT NULL - GROUP BY d.devParentMAC COLLATE NOCASE - UNION - SELECT DISTINCT 'devParentRelType' AS columnName, devParentRelType AS columnValue, devParentRelType AS columnLabel - FROM Devices WHERE devParentRelType NOT IN ({NULL_EQUIVALENTS_SQL}) AND devParentRelType IS NOT NULL - UNION - SELECT DISTINCT 'devSSID' AS columnName, devSSID AS columnValue, devSSID AS columnLabel - FROM Devices WHERE devSSID NOT IN ({NULL_EQUIVALENTS_SQL}) AND devSSID IS NOT NULL - ORDER BY columnName; - """ +sql_devices_filters = build_devices_filters_sql(NULL_EQUIVALENTS_SQL) sql_devices_stats = f""" SELECT diff --git a/server/db/device_filter_columns.py b/server/db/device_filter_columns.py new file mode 100644 index 000000000..d92616edc --- /dev/null +++ b/server/db/device_filter_columns.py @@ -0,0 +1,64 @@ +""" +Single source of truth for which Devices-table columns are filterable in the +UI, used by both the generated sql_devices_filters query (server/const.py) +and the ui_settings plugin's columns_filters.options[] (synced via +sync_device_filter_columns_config.py). Follows the same dict-registry + +generator pattern as schema_columns.py. + +Registry order is preserved into columns_filters.options[] by the sync +script, so it matches the order that setting's options have always been +hand-maintained in - reordering this dict changes the options list order +the Settings page's filter picker presents. + +devFlapping is intentionally absent: it's computed in DevicesView's CTE, +not a plain Devices column, so the generic "FROM Devices" block shape this +registry drives cannot reach it without a structural change to the +generator. See device-filter-column-registry.md for the full design. +""" + +DEVICE_FILTER_COLUMNS = { + "devOwner": {"label_key": "Device_TableHead_Owner"}, + "devType": {"label_key": "Device_TableHead_Type"}, + "devGroup": {"label_key": "Device_TableHead_Group"}, + "devLocation": {"label_key": "Device_TableHead_Location"}, + "devVendor": {"label_key": "Device_TableHead_Vendor"}, + "devSyncHubNode": {"label_key": "Device_TableHead_SyncHubNodeName"}, + "devSite": {"label_key": "Device_TableHead_NetworkSite"}, + "devSSID": {"label_key": "Device_TableHead_SSID"}, + "devSourcePlugin": {"label_key": "Device_TableHead_SourcePlugin"}, + "devParentRelType": {"label_key": "Device_TableHead_ParentRelType"}, + "devParentMAC": {"label_key": "Device_TableHead_Parent_MAC", "label_join": "parent_name"}, + "devVlan": {"label_key": "Device_TableHead_Vlan"}, +} + + +def _generic_filter_block(column_name, null_equivalents_sql): + """Return the UNION SELECT block for an ordinary Devices column with no special label join.""" + return f"""SELECT DISTINCT '{column_name}' AS columnName, {column_name} AS columnValue, {column_name} AS columnLabel + FROM Devices WHERE {column_name} NOT IN ({null_equivalents_sql}) AND {column_name} IS NOT NULL""" + + +def _parent_mac_filter_block(null_equivalents_sql): + """Return the UNION SELECT block for devParentMAC, resolving the parent device's name as the label.""" + return f"""SELECT 'devParentMAC' AS columnName, d.devParentMAC AS columnValue, + COALESCE(p.devName, d.devParentMAC) AS columnLabel + FROM Devices d + LEFT JOIN Devices p ON LOWER(p.devMac) = LOWER(d.devParentMAC) + WHERE d.devParentMAC NOT IN ({null_equivalents_sql}) AND d.devParentMAC IS NOT NULL + GROUP BY d.devParentMAC COLLATE NOCASE""" + + +def build_devices_filters_sql(null_equivalents_sql): + """Generate the sql_devices_filters UNION query from DEVICE_FILTER_COLUMNS, one block per registry entry. + + null_equivalents_sql is const.NULL_EQUIVALENTS_SQL, passed in rather than imported to avoid a circular + import (const.py is this module's caller and defines NULL_EQUIVALENTS_SQL before calling in). + """ + blocks = [] + for column_name, spec in DEVICE_FILTER_COLUMNS.items(): + if spec.get("label_join") == "parent_name": + blocks.append(_parent_mac_filter_block(null_equivalents_sql)) + else: + blocks.append(_generic_filter_block(column_name, null_equivalents_sql)) + + return "\n UNION\n ".join(blocks) + "\n ORDER BY columnName;\n " diff --git a/server/db/sync_device_filter_columns_config.py b/server/db/sync_device_filter_columns_config.py new file mode 100644 index 000000000..d93e3a3db --- /dev/null +++ b/server/db/sync_device_filter_columns_config.py @@ -0,0 +1,52 @@ +""" +Rewrites server/plugins/ui_settings/config.json's columns_filters.options[] +to exactly match DEVICE_FILTER_COLUMNS' label_keys, in registry order. +Manual dev-time step after editing device_filter_columns.py, matching +front/php/templates/language/merge_translations.py's convention for +en_us.json - not run automatically at app startup. + +Usage: python3 server/db/sync_device_filter_columns_config.py +""" + +import json +import os +import sys + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from db.device_filter_columns import DEVICE_FILTER_COLUMNS # noqa: E402 + +CONFIG_PATH = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), + "plugins", "ui_settings", "config.json", +) + + +def sync_columns_filters_options(config_path=CONFIG_PATH): + """Rewrite columns_filters.options[] in the given ui_settings config.json to match the registry's label_keys, in order.""" + with open(config_path, "r", encoding="utf-8") as f: + config = json.load(f) + + expected_options = [spec["label_key"] for spec in DEVICE_FILTER_COLUMNS.values()] + + changed = False + for setting in config.get("settings", []): + if setting.get("function") == "columns_filters": + if setting.get("options") != expected_options: + setting["options"] = expected_options + changed = True + break + + if changed: + with open(config_path, "w", encoding="utf-8") as f: + json.dump(config, f, indent=2) + f.write("\n") + + return changed + + +if __name__ == "__main__": + if sync_columns_filters_options(): + print("columns_filters.options[] updated to match DEVICE_FILTER_COLUMNS.") + else: + print("columns_filters.options[] already matches DEVICE_FILTER_COLUMNS - no change.") diff --git a/test/db/test_device_filter_columns.py b/test/db/test_device_filter_columns.py new file mode 100644 index 000000000..07b1830ac --- /dev/null +++ b/test/db/test_device_filter_columns.py @@ -0,0 +1,153 @@ +""" +Tests for the Devices filterable-columns registry (server/db/device_filter_columns.py) +and its two generated consumers: const.sql_devices_filters and ui_settings/config.json's +columns_filters.options[] - see device-filter-column-registry.md. + +Four things are tested: +1. build_devices_filters_sql() produces correct query results for both the generic + column shape and the devParentMAC special-case shape, and covers every registry + column. +2. Every registry label_key exists as a real key in en_us.json. +3. ui_settings/config.json's columns_filters.options[] exactly matches the registry's + label_keys, in registry order (the drift guard - fails if someone edits the + registry and forgets to run the sync script). +4. sync_device_filter_columns_config.py is idempotent and a no-op against the + current (already-synced) config.json. +""" + +import json +import os +import sys + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server")) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +from const import NULL_EQUIVALENTS_SQL # noqa: E402 +from db.device_filter_columns import DEVICE_FILTER_COLUMNS, build_devices_filters_sql # noqa: E402 +from db.sync_device_filter_columns_config import sync_columns_filters_options # noqa: E402 +from db_test_helpers import make_db, insert_device_from_dict, make_device_dict # noqa: E402 + +_EN_US_JSON_PATH = os.path.join( + os.path.dirname(__file__), "..", "..", "front", "php", "templates", "language", "en_us.json" +) +_UI_SETTINGS_CONFIG_PATH = os.path.join( + os.path.dirname(__file__), "..", "..", "server", "plugins", "ui_settings", "config.json" +) + + +class TestGeneratorCorrectness: + def test_generic_block_returns_right_rows(self): + """A column with no label_join resolves columnValue/columnLabel to its own value.""" + conn = make_db() + insert_device_from_dict(conn, make_device_dict("aa:bb:cc:dd:ee:01", devOwner="Alice")) + insert_device_from_dict(conn, make_device_dict("aa:bb:cc:dd:ee:02", devOwner="")) + + sql = build_devices_filters_sql(NULL_EQUIVALENTS_SQL) + rows = conn.execute(sql).fetchall() + owner_rows = [r for r in rows if r["columnName"] == "devOwner"] + + assert len(owner_rows) == 1 + assert owner_rows[0]["columnValue"] == "Alice" + assert owner_rows[0]["columnLabel"] == "Alice" + + def test_parent_mac_block_resolves_parent_name_as_label(self): + """devParentMAC's columnLabel is the parent device's devName, not its MAC.""" + conn = make_db() + insert_device_from_dict(conn, make_device_dict("aa:bb:cc:dd:ee:01", devName="Router", devParentMAC="")) + insert_device_from_dict(conn, make_device_dict( + "aa:bb:cc:dd:ee:02", devName="Laptop", devParentMAC="aa:bb:cc:dd:ee:01", + )) + + sql = build_devices_filters_sql(NULL_EQUIVALENTS_SQL) + rows = conn.execute(sql).fetchall() + parent_mac_rows = [r for r in rows if r["columnName"] == "devParentMAC"] + + assert len(parent_mac_rows) == 1 + assert parent_mac_rows[0]["columnValue"] == "aa:bb:cc:dd:ee:01" + assert parent_mac_rows[0]["columnLabel"] == "Router" + + def test_parent_mac_block_falls_back_to_mac_when_parent_unknown(self): + """If the parent MAC doesn't match any Devices row, columnLabel falls back to the raw MAC.""" + conn = make_db() + insert_device_from_dict(conn, make_device_dict( + "aa:bb:cc:dd:ee:02", devParentMAC="aa:bb:cc:dd:ee:99", + )) + + sql = build_devices_filters_sql(NULL_EQUIVALENTS_SQL) + rows = conn.execute(sql).fetchall() + parent_mac_rows = [r for r in rows if r["columnName"] == "devParentMAC"] + + assert len(parent_mac_rows) == 1 + assert parent_mac_rows[0]["columnLabel"] == "aa:bb:cc:dd:ee:99" + + def test_every_registry_column_appears_in_generated_sql(self): + """Every DEVICE_FILTER_COLUMNS entry has a block in the generated SQL - catches a + registry entry silently dropped by the generator loop.""" + sql = build_devices_filters_sql(NULL_EQUIVALENTS_SQL) + for column_name in DEVICE_FILTER_COLUMNS: + assert f"'{column_name}' AS columnName" in sql, f"{column_name} missing from generated SQL" + + +class TestRegistryToLanguageFileConsistency: + def test_every_label_key_exists_in_en_us_json(self): + with open(_EN_US_JSON_PATH, encoding="utf-8") as f: + en_us = json.load(f) + + for column_name, spec in DEVICE_FILTER_COLUMNS.items(): + assert spec["label_key"] in en_us, ( + f"{column_name}'s label_key {spec['label_key']!r} is missing from en_us.json" + ) + + +class TestRegistryToConfigDriftGuard: + def test_config_options_match_registry_in_order(self): + with open(_UI_SETTINGS_CONFIG_PATH, encoding="utf-8") as f: + config = json.load(f) + + columns_filters = next( + s for s in config["settings"] if s.get("function") == "columns_filters" + ) + expected = [spec["label_key"] for spec in DEVICE_FILTER_COLUMNS.values()] + assert columns_filters["options"] == expected, ( + "ui_settings/config.json's columns_filters.options[] is out of sync with " + "DEVICE_FILTER_COLUMNS - run server/db/sync_device_filter_columns_config.py" + ) + + +class TestSyncScriptIdempotency: + def test_no_diff_against_current_already_synced_config(self, tmp_path): + """Running the sync script against the real, already-correct config.json + produces no change - proves the sync script's output format matches the + hand-maintained array's existing format exactly, not just its content.""" + tmp_config = tmp_path / "config.json" + tmp_config.write_text( + open(_UI_SETTINGS_CONFIG_PATH, encoding="utf-8").read(), encoding="utf-8" + ) + + changed = sync_columns_filters_options(str(tmp_config)) + + assert changed is False + assert tmp_config.read_text(encoding="utf-8") == open( + _UI_SETTINGS_CONFIG_PATH, encoding="utf-8" + ).read() + + def test_running_twice_is_stable(self, tmp_path): + """A second run after a real change produces no further diff.""" + tmp_config = tmp_path / "config.json" + with open(_UI_SETTINGS_CONFIG_PATH, encoding="utf-8") as f: + data = json.load(f) + columns_filters = next( + s for s in data["settings"] if s.get("function") == "columns_filters" + ) + columns_filters["options"] = list(reversed(columns_filters["options"])) + tmp_config.write_text(json.dumps(data, indent=2), encoding="utf-8") + + first_run_changed = sync_columns_filters_options(str(tmp_config)) + content_after_first_run = tmp_config.read_text(encoding="utf-8") + + second_run_changed = sync_columns_filters_options(str(tmp_config)) + content_after_second_run = tmp_config.read_text(encoding="utf-8") + + assert first_run_changed is True + assert second_run_changed is False + assert content_after_first_run == content_after_second_run From 2e2462c28008cbbfaab5d812dbc282247e299f27 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Mon, 5 Oct 2026 11:01:41 +1100 Subject: [PATCH 13/13] DOCS: skills --- .claude/skills/database-patterns/SKILL.md | 10 ++++++++++ .claude/skills/scan-pipeline/SKILL.md | 2 ++ .gemini/skills/database-patterns/SKILL.md | 10 ++++++++++ .gemini/skills/scan-pipeline/SKILL.md | 2 ++ .github/skills/database-patterns/SKILL.md | 10 ++++++++++ .github/skills/scan-pipeline/SKILL.md | 2 ++ CLAUDE.md | 1 + 7 files changed, 37 insertions(+) diff --git a/.claude/skills/database-patterns/SKILL.md b/.claude/skills/database-patterns/SKILL.md index 5b3891cfd..592f5e85c 100644 --- a/.claude/skills/database-patterns/SKILL.md +++ b/.claude/skills/database-patterns/SKILL.md @@ -34,6 +34,16 @@ Before implementing any feature that reads or writes the `Devices` table, audit --- +## Device Identity: `devMac` (today's PK) vs `devGUID` (the intended durable identity) + +`devMac STRING(50) PRIMARY KEY NOT NULL COLLATE NOCASE` (`server/db/schema/app.sql`) is still the literal SQL primary key. `devGUID TEXT` (indexed via `idx_dev_guid`) is a plain column today, but per the maintainer it's the intended long-term durable identity, since MAC has known limits as an identifier that `devGUID` doesn't share (privacy MAC randomization on iOS/Android/Windows, virtualized/containerized interfaces sharing one physical MAC, multi-homed devices presenting several). `devGUID` already backs device-history grouping (`server/models/device_history_instance.py`) and workflow trigger lookups (`server/workflows/triggers.py`). + +This is a gradual, in-progress migration, not a flag day. New code should resolve device identity from an already-fetched device row (which carries both `devMac` and `devGUID`) rather than assuming either field is *the* identifier, so it doesn't need rework as the migration progresses. + +**One thing that will never migrate, regardless of how far the PK change goes:** `Plugins_Objects.objectPrimaryId`, `CurrentScan.scanMac`, and `Events.eveMac` are permanently MAC-keyed. A plugin discovers a device by scanning the network, so it can only ever report a MAC address, never an app-internal `devGUID` NetAlertX hasn't assigned yet at scan time. This isn't a migration gap to eventually close; it's a structural ceiling on what network-originated data can ever identify a device by. + +--- + ## `*Source` Fields — Attribution System The `FIELD_SOURCE_MAP` in `server/db/authoritative_handler.py` defines 10 fields that carry write attribution via paired `*Source` columns: diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 06e48bdc1..d1a5c1333 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -58,6 +58,8 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. 8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. +7. **`CurrentScan.scanMac`/`Events.eveMac` are permanently MAC-keyed, independent of the devGUID-as-PK migration.** See `database-patterns`' "Device Identity" section - `devMac` is today's actual schema PK, `devGUID` is the intended long-term identity, but a plugin can only ever report a MAC from network discovery, never an app-internal `devGUID`. Don't design around these tables ever becoming devGUID-keyed. + ## When to read this vs. other docs/skills - Writing or reviewing a plugin's `config.json`/data contract → `plugin-development`, `docs/PLUGINS_DEV*.md`. This skill covers what happens *after* a plugin's rows land in `CurrentScan`, not the authoring contract. diff --git a/.gemini/skills/database-patterns/SKILL.md b/.gemini/skills/database-patterns/SKILL.md index 9e1ece727..bf50677e7 100644 --- a/.gemini/skills/database-patterns/SKILL.md +++ b/.gemini/skills/database-patterns/SKILL.md @@ -34,6 +34,16 @@ Before implementing any feature that reads or writes the `Devices` table, audit --- +## Device Identity: `devMac` (today's PK) vs `devGUID` (the intended durable identity) + +`devMac STRING(50) PRIMARY KEY NOT NULL COLLATE NOCASE` (`server/db/schema/app.sql`) is still the literal SQL primary key. `devGUID TEXT` (indexed via `idx_dev_guid`) is a plain column today, but per the maintainer it's the intended long-term durable identity, since MAC has known limits as an identifier that `devGUID` doesn't share (privacy MAC randomization on iOS/Android/Windows, virtualized/containerized interfaces sharing one physical MAC, multi-homed devices presenting several). `devGUID` already backs device-history grouping (`server/models/device_history_instance.py`) and workflow trigger lookups (`server/workflows/triggers.py`). + +This is a gradual, in-progress migration, not a flag day. New code should resolve device identity from an already-fetched device row (which carries both `devMac` and `devGUID`) rather than assuming either field is *the* identifier, so it doesn't need rework as the migration progresses. + +**One thing that will never migrate, regardless of how far the PK change goes:** `Plugins_Objects.objectPrimaryId`, `CurrentScan.scanMac`, and `Events.eveMac` are permanently MAC-keyed. A plugin discovers a device by scanning the network, so it can only ever report a MAC address, never an app-internal `devGUID` NetAlertX hasn't assigned yet at scan time. This isn't a migration gap to eventually close; it's a structural ceiling on what network-originated data can ever identify a device by. + +--- + ## `*Source` Fields — Attribution System The `FIELD_SOURCE_MAP` in `server/db/authoritative_handler.py` defines 10 fields that carry write attribution via paired `*Source` columns: diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index 224a5e834..908f356bf 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -58,6 +58,8 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. 8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. +7. **`CurrentScan.scanMac`/`Events.eveMac` are permanently MAC-keyed, independent of the devGUID-as-PK migration.** See `database-patterns`' "Device Identity" section - `devMac` is today's actual schema PK, `devGUID` is the intended long-term identity, but a plugin can only ever report a MAC from network discovery, never an app-internal `devGUID`. Don't design around these tables ever becoming devGUID-keyed. + ## When to read this vs. other docs/skills - Writing or reviewing a plugin's `config.json`/data contract → `plugin-development`, `docs/PLUGINS_DEV*.md`. This skill covers what happens *after* a plugin's rows land in `CurrentScan`, not the authoring contract. diff --git a/.github/skills/database-patterns/SKILL.md b/.github/skills/database-patterns/SKILL.md index f045942c7..5ce899fc3 100644 --- a/.github/skills/database-patterns/SKILL.md +++ b/.github/skills/database-patterns/SKILL.md @@ -34,6 +34,16 @@ Before implementing any feature that reads or writes the `Devices` table, audit --- +## Device Identity: `devMac` (today's PK) vs `devGUID` (the intended durable identity) + +`devMac STRING(50) PRIMARY KEY NOT NULL COLLATE NOCASE` (`server/db/schema/app.sql`) is still the literal SQL primary key. `devGUID TEXT` (indexed via `idx_dev_guid`) is a plain column today, but per the maintainer it's the intended long-term durable identity, since MAC has known limits as an identifier that `devGUID` doesn't share (privacy MAC randomization on iOS/Android/Windows, virtualized/containerized interfaces sharing one physical MAC, multi-homed devices presenting several). `devGUID` already backs device-history grouping (`server/models/device_history_instance.py`) and workflow trigger lookups (`server/workflows/triggers.py`). + +This is a gradual, in-progress migration, not a flag day. New code should resolve device identity from an already-fetched device row (which carries both `devMac` and `devGUID`) rather than assuming either field is *the* identifier, so it doesn't need rework as the migration progresses. + +**One thing that will never migrate, regardless of how far the PK change goes:** `Plugins_Objects.objectPrimaryId`, `CurrentScan.scanMac`, and `Events.eveMac` are permanently MAC-keyed. A plugin discovers a device by scanning the network, so it can only ever report a MAC address, never an app-internal `devGUID` NetAlertX hasn't assigned yet at scan time. This isn't a migration gap to eventually close; it's a structural ceiling on what network-originated data can ever identify a device by. + +--- + ## `*Source` Fields — Attribution System The `FIELD_SOURCE_MAP` in `server/db/authoritative_handler.py` defines 10 fields that carry write attribution via paired `*Source` columns: diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 344549977..09a95c033 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -58,6 +58,8 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. 8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. +7. **`CurrentScan.scanMac`/`Events.eveMac` are permanently MAC-keyed, independent of the devGUID-as-PK migration.** See `database-patterns`' "Device Identity" section - `devMac` is today's actual schema PK, `devGUID` is the intended long-term identity, but a plugin can only ever report a MAC from network discovery, never an app-internal `devGUID`. Don't design around these tables ever becoming devGUID-keyed. + ## When to read this vs. other docs/skills - Writing or reviewing a plugin's `config.json`/data contract → `plugin-development`, `docs/PLUGINS_DEV*.md`. This skill covers what happens *after* a plugin's rows land in `CurrentScan`, not the authoring contract. diff --git a/CLAUDE.md b/CLAUDE.md index c0061bb8e..2e5392972 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -93,6 +93,7 @@ Procedural/how-to knowledge (running tests, resetting the DB, devcontainer manag - 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 real translation; other locales fall back to it automatically at runtime for a key they don't have. After adding or changing any key in `en_us.json`, run `python3 front/php/templates/language/merge_translations.py` (plain stdlib, no deps) - it re-sorts `en_us.json` alphabetically and propagates the new key into every other locale file with an empty placeholder value, so translators see what needs translating. Skipping this leaves the other 23 locale files out of sync with `en_us.json`'s key set. - A filterable Devices-table column is added in one place: `DEVICE_FILTER_COLUMNS` (`server/db/device_filter_columns.py`), which generates both `sql_devices_filters` (`server/const.py`) and (after running `python3 server/db/sync_device_filter_columns_config.py`) `server/plugins/ui_settings/config.json`'s `columns_filters.options[]`. Never hand-edit either generated side directly - `test/db/test_device_filter_columns.py`'s drift guard fails if the registry and `config.json` disagree. This is for the *filterable*-columns list only; the separate *displayable*-columns list (`device_columns.options[]`, `front/js/device-columns.js`) is untouched by this mechanism. - **Search before you build.** Before writing a new check/condition/helper for something (an "is X true" computation, a UI state signal, a utility), search the codebase for an existing implementation of the same or similar logic first - the same file (read the whole file, not just the section being edited), a sibling page, the Python backend. If one exists, extract it into a shared function and call it from the new site; don't write a parallel implementation planning to deduplicate later. A real case: a new frontend indicator needed to know "is the backend still applying a settings change" - that exact check already existed inline in `settings.php`'s own polling loop (`handleLoadingDialog()`), found only after two rounds of reinventing it elsewhere (a cookie-based guess, then a duplicate PHP endpoint) instead of reading the rest of the file first. +- **Before proposing a new top-level UI surface (tab, page, nav entry), check whether an existing one already owns the same underlying data and shell and would be better served by a mode/view toggle inside it.** This is "search before you build" one level up - not "does this logic exist" but "does a container for this already exist, just grouped differently." A real case: a new field-pivoted view of `Plugins_Objects` was designed as its own new device-details tab, even after explicitly noting it was "the exact same pattern as the existing Plugins tab, just re-pivoted by field instead of plugin" - the structural-identity observation was made but not followed to its conclusion, because every other tab on that page is single-purpose with no internal mode switch, and that precedent was pattern-matched by default instead of questioned. Caught only when prompted to reconsider; the fix was a `[ Plugin View | Field View ]` toggle inside the existing tab, not a new one beside it. Don't wait to be asked - when a new view's data source and shell both match an existing surface, ask whether it's a second view of that surface before scaffolding a new one. - **No new PHP backend logic.** `front/` is being migrated away from PHP, so never add a new PHP endpoint or new server-side computation inside an existing PHP file. If a feature needs backend state or computation, add it to the Python server and expose it to the frontend through an existing read path (`app_state.json` via `query_json.php`, `table_settings.json`, a REST/GraphQL endpoint) - never re-derive logic in PHP that the Python side already knows or could easily expose. Editing existing PHP page logic (templating, bug fixes) is fine; this is about not growing the PHP surface area. - **A stateful UI feature (survives a reload, coordinates across tabs, reacts to a backend push) is a cross-cutting mechanism, not "just a UI feature."** Treat it like one before writing code: trace every existing consumer of the data it needs (e.g. everything that already reads `app_state.json`), not just the one file being edited. The visible surface looking small (a badge, an icon) says nothing about whether the state-propagation mechanism underneath already exists elsewhere. - **No JS/PHP test harness exists in `front/` — simulate before asking for a live test, every time, not after a live test fails.** Write a disposable Node/PHP script that runs the actual function(s) against realistic inputs (including inputs crossing from a different file than the one being edited) before calling a change ready to test. A diff review misses things a five-second script run catches - a renamed parameter still referenced by its old name, an ordering assumption that's wrong once two async steps are both in play.