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/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 374fbcd0e..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: @@ -26,7 +28,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 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/.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/pr-analysis/SKILL.md b/.gemini/skills/pr-analysis/SKILL.md index 88442278a..8202014db 100644 --- a/.gemini/skills/pr-analysis/SKILL.md +++ b/.gemini/skills/pr-analysis/SKILL.md @@ -47,9 +47,9 @@ 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). +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. ## Reply Guidelines diff --git a/.gemini/skills/prd-writing/SKILL.md b/.gemini/skills/prd-writing/SKILL.md index 4f873f9cd..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: @@ -26,7 +28,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 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/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/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/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/pr-analysis/SKILL.md b/.github/skills/pr-analysis/SKILL.md index 6251ae55d..a9aaa2513 100644 --- a/.github/skills/pr-analysis/SKILL.md +++ b/.github/skills/pr-analysis/SKILL.md @@ -47,9 +47,9 @@ 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). +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. ## Reply Guidelines diff --git a/.github/skills/prd-writing/SKILL.md b/.github/skills/prd-writing/SKILL.md index 5c052e50d..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: @@ -26,7 +28,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 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/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 87c774b29..2e5392972 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,3 +91,9 @@ 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. +- **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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a24532617..80d571dd6 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 appreciated! 💙 diff --git a/README.md b/README.md index ecf2b2274..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:** 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/) for detailed instructions - it lists each migration scenario by version, so pick the one matching your installed version. 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/). 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/css/app.css b/front/css/app.css index b4fe82bc7..b83a15ecb 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/devices-table.js b/front/js/devices-table.js index c46565d73..55253c00e 100644 --- a/front/js/devices-table.js +++ b/front/js/devices-table.js @@ -496,12 +496,18 @@ 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) { - 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)); } }, 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/server/util.php b/front/php/server/util.php index 4c9449c6d..989abc23a 100755 --- a/front/php/server/util.php +++ b/front/php/server/util.php @@ -116,7 +116,7 @@ function saveSettings() if ($group == $settingGroup) { if ($dataType == 'string' ) { - $val = encode_single_quotes($settingValue); + $val = encode_python_string($settingValue); $txt .= $setKey . "='" . $val . "'\n"; } elseif ($dataType == 'integer') { $txt .= $setKey . "=" . $settingValue . "\n"; @@ -137,7 +137,7 @@ function saveSettings() // skipping __metadata entries (?) if (count($setting) > 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 ',' @@ -272,9 +272,13 @@ function getSettingValue($setKey) { } // ------------------------------------------------------------------------------------------- -function encode_single_quotes ($val) { - $result = str_replace ('\'','{s-quote}',$val); - return $result; +/** + * Encode a string for use inside a single-quoted Python literal in app.conf. + * Doubles backslashes so they round-trip unchanged, and replaces ' with the + * legacy {s-quote} placeholder that the backend converts back per use. + */ +function encode_python_string($val) { + return str_replace(['\\', '\''], ['\\\\', '{s-quote}'], $val); } // ------------------------------------------------------------------------------------------- // Helper function to send notifications via the backend API endpoint 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..f6891c6b7 100755 --- a/front/php/templates/header.php +++ b/front/php/templates/header.php @@ -185,6 +185,10 @@ + + @@ -402,6 +406,10 @@