From 2e2462c28008cbbfaab5d812dbc282247e299f27 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Mon, 5 Oct 2026 11:01:41 +1100 Subject: [PATCH] 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.