diff --git a/.claude/skills/database-patterns/SKILL.md b/.claude/skills/database-patterns/SKILL.md index c04c8633..5b3891cf 100644 --- a/.claude/skills/database-patterns/SKILL.md +++ b/.claude/skills/database-patterns/SKILL.md @@ -9,7 +9,7 @@ description: Read before designing a feature that writes to the Devices table, a Before implementing any feature that reads or writes the `Devices` table, audit ALL write paths. The table is modified from many locations — missing one path is a correctness bug. -**Known production write paths (as of 2026-07-04):** +**Known production write paths:** | File | Function | Fields written | |---|---|---| diff --git a/.claude/skills/plugin-development/SKILL.md b/.claude/skills/plugin-development/SKILL.md index fad051fa..9bf58f0b 100644 --- a/.claude/skills/plugin-development/SKILL.md +++ b/.claude/skills/plugin-development/SKILL.md @@ -33,7 +33,8 @@ server/plugins// - `_CMD`: script path. - `_RUN_TIMEOUT`: timeout in seconds — **enforced by the core plugin runner as the whole script's kill-timeout** (`server/plugin.py` passes it straight to `subprocess(..., timeout=...)`). Not a safe per-HTTP-call timeout — don't reuse it for individual network calls in a loop, or one slow call can burn the whole budget and get the process killed before it writes its result file. Two correct alternatives: `config.json`'s `"timeoutMultiplier": true` on a `params[]` entry for a config-declared, known-length loop (see `arp_scan`); `plugin_helper.per_item_timeout()` for a runtime-variable-length loop (see the `_publisher_*` plugins). - `_WATCH`: columns to watch for changes. -- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_DEV_DATA_CONTRACT.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. +- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_IMPORT_BEHAVIOR.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. +- **`dataType` and `default_value` must agree.** `dataType: "array"`/`"object"` needs a real JSON literal for `default_value` (`'["default"]'`), not a bare string (`"default"`). `setting_value_to_python_type()` (`server/helper.py`) `json.loads()`s the default at runtime; a bare string fails silently — logged, and `[]` is returned instead of your default (e.g. `devParentRelType`, `UI_theme`, `UI_TOPOLOGY_ORDER`). If `elementOptions` already sets `multiple`/`orderable: "false"`, the setting is scalar — use `dataType: "string"` instead. ## Data Contract @@ -70,7 +71,7 @@ Full column spec: `docs/PLUGINS_DEV_DATA_CONTRACT.md`. Note `helpVal1-4`/`watche ## Before Opening a PR -Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings instead of duplicating them, description length (renders in the Settings UI — keep it short), and the multi-instance settings pattern (nested array + popup-form, see `rest_import`, not a hardcoded "primary"/"secondary" pair). Most plugin PR review comments trace back to one of these, and `test/plugins/test_plugin_conventions.py` mechanically enforces the RUN-default, description-length, hardcoded-default-drift, and RUN_TIMEOUT-reuse-in-loop items — run it after touching a plugin. +Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings instead of duplicating them, description length (renders in the Settings UI — keep it short), and the multi-instance settings pattern (nested array + popup-form, see `rest_import`, not a hardcoded "primary"/"secondary" pair). Most plugin PR review comments trace back to one of these, and `test/plugins/test_plugin_conventions.py` mechanically enforces the RUN-default, description-length, hardcoded-default-drift, RUN_TIMEOUT-reuse-in-loop, and array/object dataType-default_value-mismatch items — run it after touching a plugin. ## Starting Point diff --git a/.claude/skills/plugin-readme/SKILL.md b/.claude/skills/plugin-readme/SKILL.md index a746e0a2..743ef082 100644 --- a/.claude/skills/plugin-readme/SKILL.md +++ b/.claude/skills/plugin-readme/SKILL.md @@ -21,7 +21,7 @@ Exception: call out a *specific* setting by name, in prose, only when its behavi ## Verify against the actual code first -Read `config.json` (`unique_prefix`, `plugin_type`, `data_source`, `settings`) and the plugin's script before writing anything - don't guess at mechanism from the plugin's name alone. Real bugs found this way during a past audit: `dig_scan/README.md` described the `nbtscan` utility (copy-paste from a sibling plugin); `adguard_import/README.md` was a byte-for-byte copy of `__template/README.md`, never actually written. +Read `config.json` (`unique_prefix`, `plugin_type`, `data_source`, `settings`) and the plugin's script before writing anything - don't guess at mechanism from the plugin's name alone. A README copied from a sibling plugin or left as the unedited `__template/README.md` describes the wrong plugin's behavior. ## Backfilling missing "Other info" diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md index d7b83631..3ca701ab 100644 --- a/.claude/skills/prd-writing/SKILL.md +++ b/.claude/skills/prd-writing/SKILL.md @@ -11,12 +11,12 @@ Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "cre ## 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 real, caught-in-review examples from this exact process: +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: -- A draft claimed "plugin X's rows are the cleanest case for a static presence-flag value" — reasonable-sounding, and wrong. Reading the actual script showed it already reports a live per-row state field and computes an equivalent boolean internally that was simply never wired up. The claim was never checked against the script, only against the plugin's category ("reservation-style"). -- A draft claimed "no changes needed here" for two queries when adding a new presence column, on the reasoning that they were already "inert" for the new case. True for one sub-case (a device that starts absent), false for the transition sub-case (a device going from online to newly-suppressed) — because those two queries used a different existence check than the one already patched. It was missed for a full turn, until a direct question ("does this handle the transition where a device *was* online?") forced a re-trace of the actual call graph. +- Calling a plugin "the cleanest case" for some behavior based on its category alone. Read the actual script — it may already report a live per-row signal and compute an equivalent value internally that was never wired up. +- Claiming "no changes needed" for a query based on one sub-case (e.g. a device that starts absent) without checking the transition sub-case (a device going from online to newly-suppressed) — the two sub-cases can use different existence checks, so a fix covering one can silently miss the other. -Both mistakes were plausible, well-written, and wrong. Neither would have survived actually reading the code first. +Both are plausible, well-written, and wrong. Reading the code first catches both. ## Process @@ -28,7 +28,7 @@ Both mistakes were plausible, well-written, and wrong. Neither would have surviv 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 actually 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). 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. **Evaluate performance impact against the schema that actually exists, not the schema you'd expect, and against real deployment scale, not an imagined one.** For every new or modified query: does it reuse an existing index, or does it add an unindexed lookup, a new join, or a correlated subquery? Check for real — don't assume a column is indexed just because it looks identity-like (`CurrentScan.scanMac` looked like exactly the kind of column that should have an index; grepping for `CREATE INDEX` showed this codebase never gave it one, and neither did the first draft of the design that needed it — since fixed, `idx_currentscan_scanmac` now exists in `server/db/db_upgrade.py:ensure_CurrentScan()`, so check whether a later PRD's problem is already mitigated before assuming it's new). Then multiply the per-query cost by two things: how often it runs (a full scan inside a loop that fires once is nothing; the same scan inside a cycle that reruns every few minutes forever is a standing cost, permanently), and the actual scale this project runs at — **known real production users run 10,000+ devices** (confirmed directly by the project owner, not a guess or an inference from `CLAUDE.md`'s "homelabs, MSPs, and NOCs" framing). At that scale, a `CurrentScan` populated at 2-5 rows per device (one per contributing plugin, the normal case) is routinely 20,000-50,000+ rows in a single cycle — treat that as the number to reason about, not a hypothetical upper bound reserved for some future large deployment. Concrete example from this process: implementing a new multi-source precedence rule as a correlated `EXISTS` subquery re-evaluated per candidate row reads as perfectly reasonable, passes every test at small scale, and is an accidental self-join with no index behind it at scale — the fix (add the missing index, express the aggregation as one `GROUP BY` pass instead of a per-row correlated check) had to be written into the PRD explicitly, or it would have shipped as a footgun that real 10k-device users would have hit, not a theoretical one. +9. **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` — don't assume a column is indexed just because it looks like a key (check `server/db/db_upgrade.py`/`server/db/schema/app.sql`). 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 rather than assuming either way; if it comes back unindexed, a `GROUP BY` aggregate is the usual fix. 10. **Do a dedicated final-check pass, out loud, before calling it done.** Re-read the whole document end to end and specifically check: - Did a correction made mid-document actually propagate everywhere it needed to (the Design subsection *and* Affected Files *and* Tests *and* any execution-plan summary)? A correction landing in one place and not its siblings is worse than never catching it, because now the document silently contradicts itself. - Does every "this is the cleanest/simplest real case" claim still hold up if you actually re-read that specific piece of code right now, or was it asserted by pattern-matching a name/category? Re-verify, don't re-assert. diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 9781183a..4d84cfa6 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -7,57 +7,57 @@ description: Read before modifying the scan pipeline (server/scan/session_events ## Scope -This skill covers what happens *after* a plugin's rows land in `CurrentScan` — presence computation, event generation, and session/timeline derivation. It does not cover the plugin-authoring side (manifest, data contract, settings) — see the `plugin-development` skill and `docs/PLUGINS_DEV*.md` for that. It also does not cover the general `*Source` attribution system or SQLite triggers for audit logging — see the `database-patterns` skill (Copilot tree only, as of this writing) for that; the field-authority section below is the scan-pipeline-local half of that bigger system, and the two overlap. +Covers what happens after a plugin's rows land in `CurrentScan`: presence computation, event generation, session/timeline derivation. Not plugin authoring (manifest, data contract, settings) — see `plugin-development` and `docs/PLUGINS_DEV*.md`. Not the general `*Source` attribution system or SQLite audit triggers — see `database-patterns`; the field-authority section below is the scan-pipeline-local half of that system. -## The core tables/views and their lifetimes +## Core tables and views -- **`CurrentScan`** — ephemeral scratch table. Populated by `process_plugin_events()` for any plugin whose `config.json` declares `mapped_to_table`, then fully deleted at the end of every `process_scan()` cycle (`DELETE FROM CurrentScan`). Never assume a value written into a `CurrentScan` row is readable outside the cycle it arrived in — by the time a later cycle needs to check "was this row flagged X," the row is gone. This bit a real design (see Gotcha 2 below). +- **`CurrentScan`** — ephemeral scratch table. `process_plugin_events()` populates it for any plugin whose `config.json` declares `mapped_to_table`; `process_scan()` deletes all rows at the end of every cycle. A value written to a `CurrentScan` row is not readable in a later cycle — the row is gone by then. See Gotcha 2. - **`Devices`** — persistent identity + state table. -- **`Events`** — persistent, append-only log of state-transition events (`New Device`, `Connected`, `Down Reconnected`, `Device Down`, `Disconnected`, `IP Changed`). This *is* the audit trail; `Sessions` is derived from it, not the other way around. -- **`Sessions`** — not incrementally updated; fully wiped and rebuilt every cycle from a view (see `Convert_Events_to_Sessions` below). Don't reason about it as a live connection state machine — it's a materialized query result recomputed from `Events` each cycle. -- **`Online_History`** — persistent, one row per scan cycle, feeds the dashboard's online/offline graph. Purely a rollup of `devPresentLastScan`/`devAlertDown`/`devIsSleeping` counts on `DevicesView` — no independent state of its own. +- **`Events`** — persistent, append-only log of state-transition events (`New Device`, `Connected`, `Down Reconnected`, `Device Down`, `Disconnected`, `IP Changed`). This is the audit trail; `Sessions` is derived from it, not the reverse. +- **`Sessions`** — fully wiped and rebuilt every cycle from `Convert_Events_to_Sessions` (below), not incrementally updated. Treat it as a materialized query result, not a live connection state machine. +- **`Online_History`** — one row per scan cycle, feeds the dashboard's online/offline graph. A rollup of `devPresentLastScan`/`devAlertDown`/`devIsSleeping` counts on `DevicesView` — no state of its own. ## Key views -- **`LatestDeviceScan`** (`server/db/db_upgrade.py`) — `Devices` LEFT JOIN'd to the most recent `CurrentScan` row **per `(scanMac, scanSourcePlugin)` pair**, ranked via `ROW_NUMBER() OVER (PARTITION BY scanMac, scanSourcePlugin ...)`. This is why `update_devices_data_from_scan()` loops over `DISTINCT scanSourcePlugin` and re-queries this view once per plugin: when two plugins report the same device in the same cycle, they are *not* merged into one row before processing — each plugin's contribution is evaluated independently, per field, through the authority mechanism below. -- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan`. Used by the "New Connections" query in `insert_events()` to decide whether a device was previously down (→ `Down Reconnected`) or genuinely new (→ `Connected`). -- **`Convert_Events_to_Sessions`** — the actual definition of "is this device's session still open." **There is no `close_session()`-style function anywhere in this codebase.** A session closes purely as an emergent property: `pair_sessions_events()` sets `evePairEventRowid` on a `New Device`/`Connected`/`Down Reconnected` Event to point at the next `Disconnected`/`Device Down` Event for that MAC, and this view computes `sesStillConnected = 1` exactly when that pairing is still `NULL`. If a session needs to close, the fix is always "make sure the right `Events` row gets inserted" — never a direct `Sessions` mutation (the one exception is `create_new_devices()`'s reconnect-insert, noted below). -- **`DevicesView`** — adds computed `devIsSleeping`/`devFlapping`/`devStatus` on top of `Devices`. This is what the UI and `insertOnlineHistory()` actually read presence from, not the raw `Devices` table. +- **`LatestDeviceScan`** (`server/db/db_upgrade.py`) — `Devices` LEFT JOIN'd to the most recent `CurrentScan` row per `(scanMac, scanSourcePlugin)` pair, via `ROW_NUMBER() OVER (PARTITION BY scanMac, scanSourcePlugin ...)`. `update_devices_data_from_scan()` loops over `DISTINCT scanSourcePlugin` and re-queries this view once per plugin: when two plugins report the same device in one cycle, each contribution is evaluated separately, per field, through the authority mechanism below — they are not merged into one row first. +- **`LatestEventsPerMAC`** — most recent Event per MAC, joined to `Devices` and `CurrentScan`. The "New Connections" query in `insert_events()` uses it to decide whether a device was previously down (→ `Down Reconnected`) or new (→ `Connected`). +- **`Convert_Events_to_Sessions`** — defines "is this device's session still open." There is no `close_session()` function anywhere in this codebase. A session closes as an emergent property: `pair_sessions_events()` sets `evePairEventRowid` on a `New Device`/`Connected`/`Down Reconnected` Event to point at the next `Disconnected`/`Device Down` Event for that MAC; this view sets `sesStillConnected = 1` exactly when that pairing is `NULL`. To close a session, insert the right `Events` row — never mutate `Sessions` directly (the one exception is `create_new_devices()`'s reconnect-insert, in the call order below). +- **`DevicesView`** — adds computed `devIsSleeping`/`devFlapping`/`devStatus` on top of `Devices`. The UI and `insertOnlineHistory()` read presence from this, not the raw `Devices` table. -## `process_scan()` call order (`server/scan/session_events.py`) — the order is load-bearing, not incidental +## `process_scan()` call order (`server/scan/session_events.py`) — order is load-bearing 1. `save_own_device()`, `exclude_ignored_devices()` -2. `insert_events(db)` — **runs before presence gets updated for this cycle.** Deliberate: the Down/Disconnected/Connected queries need the *previous* cycle's `devPresentLastScan` value to detect a transition (present last cycle but absent now, or vice versa). If this ran after the presence update, every query would see the already-updated value and the edge-triggered design would collapse into either never firing or firing every cycle. -3. `create_new_devices(db)` — the source comment is explicit: "after create events -> avoid 'connection' event." Brand-new devices get a `New Device` event instead of a `Connected` event, because at step 2 they didn't exist as `Devices` rows for either query to match. Also contains a raw `INSERT INTO Sessions ... sesStillConnected = 1` for devices that already exist but have no currently-open session — the one place outside the `Events`-derived path that writes `Sessions` directly. +2. `insert_events(db)` — runs before presence updates for this cycle. The Down/Disconnected/Connected queries need the *previous* cycle's `devPresentLastScan` to detect a transition. If this ran after the presence update, every query would see the new value and the edge-triggered design would break — firing never, or every cycle. +3. `create_new_devices(db)` — runs before presence updates so a brand-new device gets a `New Device` event, not a `Connected` event (it has no `Devices` row yet for step 2's queries to match). Also has a raw `INSERT INTO Sessions ... sesStillConnected = 1` for existing devices with no open session — the one place outside the `Events`-derived path that writes `Sessions` directly. 4. `update_devices_data_from_scan(db)` — field-level updates for existing devices; see the authority mechanism below. 5. `update_sync_hub_node`, `update_devLastConnection_from_CurrentScan` -6. `update_presence_from_CurrentScan(db)` — sets `devPresentLastScan` from bare `CurrentScan` presence *for this cycle* (this becomes the "previous" value step 2 reads on the *next* cycle). -7. `update_devPresentLastScan_based_on_nics(db)` — NIC/parent-child presence aggregation; can override step 6's result for parent devices. -8. `update_devPresentLastScan_based_on_force_status(db)` — the user's manual `devForceStatus` override, runs **last**, wins unconditionally over everything above. +6. `update_presence_from_CurrentScan(db)` — sets `devPresentLastScan` from `CurrentScan` for this cycle (step 2 reads this as "previous" on the *next* cycle). +7. `update_devPresentLastScan_based_on_nics(db)` — NIC/parent-child presence aggregation; can override step 6 for parent devices. +8. `update_devPresentLastScan_based_on_force_status(db)` — the user's manual `devForceStatus` override; runs last, wins over everything above. 9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` 10. `pair_sessions_events(db)` — pairs `Events` rows as described above. -11. `create_sessions_snapshot(db)` — `DELETE FROM Sessions; INSERT INTO Sessions SELECT * FROM Convert_Events_to_Sessions`. This is the point where `Sessions` actually reflects step 10's pairing. +11. `create_sessions_snapshot(db)` — `DELETE FROM Sessions; INSERT INTO Sessions SELECT * FROM Convert_Events_to_Sessions`. `Sessions` reflects step 10's pairing from here. 12. `insertOnlineHistory(db)` — dashboard graph rollup. 13. `skip_repeated_notifications(db)` -14. `DELETE FROM CurrentScan` — the ephemeral table's entire lifetime is one call to `process_scan()`. +14. `DELETE FROM CurrentScan` — the table's entire lifetime is one call to `process_scan()`. ## Field-write authority for scan-derived updates -`update_devices_data_from_scan()` (`server/scan/device_handling.py`) does not blindly overwrite fields from whichever plugin ran most recently. Each trackable field is declared once in `FIELD_SPECS` (`scan_col`, `source_col`, a `priority` list of plugin prefixes, optional `allow_override_if_changed`), and `can_overwrite_field()` uses that plus `get_plugin_authoritative_settings()` (which reads a plugin's own settings for an explicit authority override) to decide, per field per row, whether this plugin's value may replace what's there. The paired `Source` column (`devNameSource`, `devLastIPSource`, etc.) records who currently owns the field. `devMac` itself is never a target of these updates — it's the join key, not a tracked field — so no scan-derived update path can ever alter a device's identity, only its attributes. +`update_devices_data_from_scan()` (`server/scan/device_handling.py`) does not overwrite fields from whichever plugin ran most recently. Each trackable field is declared once in `FIELD_SPECS` (`scan_col`, `source_col`, a `priority` list of plugin prefixes, optional `allow_override_if_changed`). `can_overwrite_field()` uses that plus `get_plugin_authoritative_settings()` (a plugin's own authority-override setting, if any) to decide, per field per row, whether this plugin's value may replace what's there. The paired `Source` column (`devNameSource`, `devLastIPSource`, etc.) records who currently owns the field. `devMac` is never a target of these updates — it's the join key, not a tracked field — so no scan-derived update can alter a device's identity, only its attributes. -This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. +This is the scan-pipeline-local half of a bigger attribution system — see `database-patterns` for `FIELD_SOURCE_MAP`/`server/db/authoritative_handler.py`, the full `*Source` model, and the SQLite triggers that consume it for audit logging. Read both before touching anything that writes a `*Source` column. -## Four real gotchas (not hypothetical — all surfaced live during a design review) +## Gotchas -1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. -2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. -3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. -4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. - -**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. **`Events`, `Sessions`, and `Notifications`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. +1. **A "presence" check exists in more than one place.** A per-row signal meaning "don't count this as a live sighting" (e.g. `scanPresence`) has to reach every query that independently re-derives "is this MAC currently present" from `CurrentScan`. `current_scan_presence_condition()` (`server/scan/presence.py`) centralizes that check for five sites: `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`). Two sites can't use it: the "New Connections" query and the raw `Sessions` insert in `create_new_devices()` need the actual `scanLastIP`/`scanVendor` value off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean. Check any new presence-adjacent query against both patterns — a bare helper call isn't always enough. +2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it can't express a decision that needs to survive to a cycle where the row is gone.** Anything that fires because a row is *missing* (`Device Down`, `Disconnected`) can't read a flag that lived on that row. A per-row plugin signal that needs to affect behavior beyond its own cycle has to persist onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults), not ride on the ephemeral table. +3. **`CurrentScan` is not small, and it's indexed on `scanMac`.** Real production users run 10,000+ devices; with one row per contributing plugin (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows. `idx_currentscan_scanmac` (`server/db/db_upgrade.py:ensure_CurrentScan()`, mirrored in `server/db/schema/app.sql`) covers every `scanMac`-keyed lookup in this file. `ensure_CurrentScan()`'s `DROP TABLE`/`CREATE TABLE` runs once, at app startup (`DB.initDB()`, `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1, which clears rows but leaves the table and its index in place. +4. **`server/plugins/sync/sync.py` bypasses this pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode` awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both skip `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments: "Node is fully authoritative in this mode"). It's a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it like any other plugin — but its two direct-write paths ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0`. Don't assume every `Events`/`Devices` write goes through the generic pipeline — `sync.py` doesn't. +5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. +6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. ## When to read this vs. other docs/skills -- Writing or reviewing a plugin's `config.json`/data contract → `plugin-development` skill, `docs/PLUGINS_DEV*.md`. This skill is about what happens *after* a plugin's rows land in `CurrentScan`, not the plugin-authoring contract itself. -- Devices-table write paths, `*Source` attribution, audit/history logging, SQLite triggers → `database-patterns` skill. -- Actually implementing a change here → read the relevant function in `server/scan/session_events.py` / `server/scan/device_handling.py` directly before trusting this skill's line-number references; they're a map, not a guarantee, and will drift as the code moves. +- 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. +- Devices-table write paths, `*Source` attribution, audit/history logging, SQLite triggers → `database-patterns`. +- Implementing a change here → read the actual function in `server/scan/session_events.py`/`server/scan/device_handling.py` first; this skill's line numbers are a map, not a guarantee, and drift as the code moves. diff --git a/.claude/skills/skill-hygiene/SKILL.md b/.claude/skills/skill-hygiene/SKILL.md new file mode 100644 index 00000000..7befeabe --- /dev/null +++ b/.claude/skills/skill-hygiene/SKILL.md @@ -0,0 +1,44 @@ +--- +name: skill-hygiene +description: Read before writing or editing any SKILL.md in this repo (.claude/.gemini/.github skills trees). Covers the two standing rules for skill prose - state current behavior only, and prefer plain, short wording - plus the grep sweep to run before calling a skill clean. +--- + +# Skill Hygiene + +## What a skill is, and isn't + +A skill is a live reference for how the system works *now*. It is not a lab notebook, a changelog, or a PRD. History, corrections, and discovery narratives belong in PRDs (`.gemini/internal-docs/PRDs/`, see `prd-writing`) or commit messages — never in a skill body. If a skill needs to change because the code changed, edit it in place; don't leave a trace of what it used to say. + +This is a different rule from `prd-writing`'s "leave a visible trail of corrections" — that rule is specifically for PRDs, where the trail is the point. A skill has no reader who benefits from seeing your editing history; it only has readers who need the current fact, stated plainly. + +## Rule 1: state current behavior only + +Cut anything that narrates the past instead of stating the present: + +| Smell | Why it's a problem | Fix | +|---|---|---| +| `"Correction: X — an earlier version of this note said Y"` | Narrates an edit, not a fact | Just state the current fact about X | +| `"(as of 2026-07-04)"` | Hedges instead of committing to the fact | State it as true now; update the line when it stops being true | +| `"This shipped for real in devParentRelType"` / `"...caught in review"` / `"...caught mid-review"` | Tells the story of finding a bug instead of the rule that prevents it | State the rule; use the real example as a plain parenthetical if it helps, without the origin story | +| `"Two real examples from this exact process:"` / `"during a past audit"` | Frames the skill as a diary of one session | Turn the anecdote into a timeless illustration, or drop it | +| `"X now does Y"` / `"X previously did Y"` / `"used to be"` / `"no longer"` when describing *the skill's own past text* | Describes the skill's edit history, not the system | Delete — say what's true now, full stop | + +`"no longer"` describing real *system* behavior (e.g. "there is no longer a retry loop here") is fine — that's a fact about the code, not about the skill. The test is: does this sentence describe the codebase, or does it describe a previous version of this document? + +## Rule 2: plain words, fewer words + +If a shorter or simpler phrasing says the same thing, use it. Cut qualifiers that don't change the meaning ("actually," "really," "genuinely," "in this exact process"). Prefer a plain verb over a nominalization. A dense skill with real information beats a padded one — trim narration and hedging before trimming facts. + +## Sweep before calling a skill clean + +Run this across `.claude/skills/`, `.gemini/skills/`, `.github/skills/` (or a single file being edited): + +```bash +grep -rniE "as of 202|caught in review|caught mid-review|correction:|correction \(|shipped for real|previously|used to be|no longer|originally|was later|historically|in the past|distilled from|real mistake|it turned out|turns out|discovered that|during a past" .claude/skills/ .gemini/skills/ .github/skills/ +``` + +Read every hit in context — some are legitimate (a rule instructing PRD authors to write correction trails, or "previously down" describing device state, are not violations). Fix the ones that narrate the skill's own history instead of the system's current behavior. + +## Keep the three trees in sync + +Most skills exist as three near-identical copies (`.claude/skills//SKILL.md`, `.gemini/skills//SKILL.md`, `.github/skills//SKILL.md` — see `.gemini/skills/skills-index/SKILL.md` for the pairing map). When a hygiene fix changes a skill's body, apply the same fix to all paired copies so they stay identical (frontmatter `name`/`description` may differ per tree's own convention; the body should not). `scripts/check_skill_pairs.py`'s `GROUPS` list only flags when some-but-not-all paired files changed in a diff — it doesn't check the bodies actually match, so a manual diff after editing is still worth it. diff --git a/.claude/skills/ux-design-patterns/SKILL.md b/.claude/skills/ux-design-patterns/SKILL.md new file mode 100644 index 00000000..355691b6 --- /dev/null +++ b/.claude/skills/ux-design-patterns/SKILL.md @@ -0,0 +1,35 @@ +--- +name: ux-design-patterns +description: Read before adding or changing any front/ UI element - a control, layout, button, or interaction pattern. Covers the don't-invent-new-UX-without-a-PRD rule and the priority order for design tradeoffs (existing behavior > intuitiveness > information density > usability > utility > uniqueness > industry practices > generic UI). +--- + +# UX / Frontend Design Patterns + +## Core principle: reuse before inventing + +Don't introduce new UX behavior or visual patterns unless a PRD explicitly calls for it. Before building any new UI element, search the existing frontend for a pattern that already solves this exact need, and reuse its markup/CSS/behavior instead of inventing a new one. + +Real, recent example: a presence-page Prev/Next pager was first built with custom `