DOCS: skill updates

.github/skills/plugin-review/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: netalertx-plugin-review
-description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers the one check not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method - plus a worked real-PR example.
+description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers two checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, and suspicious/attacker-influenced plugin data not being logged - plus worked real-PR examples.
 ---
 
 # Plugin Review
@@ -20,8 +20,8 @@ Plugin scripts write their results to `RESULT_FILE` via `plugin_helper.Plugin_Ob
 ## Review flow for a raw SQL query in a plugin
 
 1. **Does an existing model method already do this?** Check the relevant `server/models/*_instance.py` file (`DeviceInstance`, `EventInstance`, `PluginObjectInstance`, etc.) before assuming one needs to be added.
-2. **If not, is it worth adding one** (`server/models/device_instance.py` etc.), or is this genuinely a one-off maintenance/schema query that belongs in the core-plugin exception list above?
-3. **Check the collation the query relies on** against the column's actual schema (`server/db/schema/app.sql`) rather than assuming — `devMac`/`eveMac`/`sesMac`/`scanMac`/`devParentMAC` are declared `COLLATE NOCASE` at the column level, so an explicit `COLLATE NOCASE` against one of them in a new query is redundant (harmless, but a sign the author didn't check). `devName` has **no** column-level collation — an explicit `COLLATE NOCASE` there is genuinely necessary if case-insensitive name matching is intended, not a mistake.
+2. **If not, is it worth adding one** (`server/models/device_instance.py` etc.), or is this a one-off maintenance/schema query that belongs in the core-plugin exception list above?
+3. **Check the collation the query relies on** against the column's actual schema (`server/db/schema/app.sql`) rather than assuming — `devMac`/`eveMac`/`sesMac`/`scanMac`/`devParentMAC` are declared `COLLATE NOCASE` at the column level, so an explicit `COLLATE NOCASE` against one of them in a new query is redundant (harmless, but a sign the author didn't check). `devName` has **no** column-level collation — an explicit `COLLATE NOCASE` there is necessary if case-insensitive name matching is intended, not a mistake.
 4. **Parameterization** — `?` placeholders, never string-formatted values into the query (this part is usually already fine; flag it if not).
 
 ## Worked example: PR #1788 (DOCKERDISC plugin)
@@ -32,3 +32,13 @@ Two raw queries in `server/plugins/dockerdisc/script.py`:
 - `resolve_host_mac()`: `SELECT devMac FROM Devices WHERE devName = ? COLLATE NOCASE` — a name lookup with real 0/1/many-match handling (falls back to a manually-configured MAC on ambiguity or no match). No existing method covers this. `devName` has no column-level collation, so the explicit `COLLATE NOCASE` here is correct, not redundant. **Fix: add `DeviceInstance.getAllByName(name)` returning every match** (not just one — the plugin's own ambiguity detection needs the full set), and have the plugin call that instead.
 
 This is the shape of the fix in general: an existence/single-row check usually already has a model method; a query with plugin-specific result handling (ambiguity, filtering) usually needs a small new method added rather than a workaround in the plugin itself.
+
+## The second check this skill adds: suspicious/attacker-influenced plugin data must be logged
+
+A plugin that parses data from an unauthenticated peer (DHCP options, mDNS/Avahi records, NetBIOS name-service responses, SSDP/UPnP, any broadcast/discovery protocol) is trusting the network, not the device it's nominally scanning — any device on the segment can answer. When such a value gets rejected, sanitized, or otherwise flagged as suspicious/malformed, that's a security-relevant event, not routine parsing noise: **is it logged?**
+
+At minimum, every such detection needs `mylog("none", ...)` (`logger.py`'s `debugLevels` — `"none"` is level 0, the always-shown floor, not filtered out at any configured `LOG_LEVEL` — matching how this codebase already logs real errors, e.g. `mylog("none", f"[Plugins] ⚠ ERROR: {e}")`). A silently-dropped or silently-mangled value with no log trace is the finding to raise — an admin investigating "why does this device's name look wrong" or "was my network probed" has nothing to go on otherwise.
+
+**A user-facing alert (`write_notification()`, `server/messaging/in_app.py`) is a separate, materially bigger decision — don't require it as a blocking condition the way the log line is.** It's persistent and unprompted, and (per existing precedent — `api_server_start.py`'s unauthorized-access-attempt alert fires unconditionally, with no rate-limiting anywhere in this codebase) a repeat offender re-sending the same payload every scan cycle can spam it indefinitely unless the PR explicitly ties it to `process_plugin_events()`'s existing `"new"`/`"watched-changed"`/`"watched-not-changed"` per-object status (`server/plugin.py:769-791`) to get repeat-suppression for free. If a PR adds `write_notification()` for this without that gating (or an equivalent), that's the thing to flag — not the absence of a user-facing alert on its own.
+
+**Worked example (generalized):** a plugin parses an unauthenticated broadcast-protocol response (e.g. a DHCP option, an mDNS/NetBIOS record) and copies a field from it verbatim into a stored value with no validation. The fix centralizes both the sanitization *and* the `mylog("none", ...)` call in one shared, plugin-agnostic enforcement point (`plugin_object_class.__init__`, `server/plugin.py`) rather than leaving individual plugin authors to remember either — the same reasoning as the raw-SQL check above: a check that depends on every plugin author independently thinking to add it will eventually ship without it.

.github/skills/prd-writing/SKILL.md

@@ -22,20 +22,21 @@ Both are plausible, well-written, and wrong. Reading the code first catches both
 
 1. **Understand the current mechanism by reading the actual code before writing anything.** Cite `file:line` for every claim about current behavior. Delegate to an Explore/general-purpose agent for breadth if the surface area is large, but treat its findings as a starting point to spot-check, not a finished citation — verify anything load-bearing yourself before it goes in the PRD. The same applies to a prior audit or PRD this one continues from: re-read its full detail section for the specific finding, not just a one-line summary-table row, before citing or extending it — a summary row can omit a caveat ("already indexed," "already fixed elsewhere") that only the detail text states, and citing the row alone can reintroduce a claim the detail text already corrected.
 2. **Challenge the idea before designing it.** If the user proposes a solution, ask: is this solving the right problem? Does it conflate unrelated concerns (see axis-separation, next)? Does a similar or previously-rejected mechanism already exist that this would collide with semantically? A naming near-collision with an existing field/concept that has different, incompatible semantics is a signal to stop and check precedence rules, not a coincidence to wave off.
-3. **Identify the independent axes.** A feature request that arrives as "option A and option B" is often two or three orthogonal concerns bundled together — e.g. "should this exist at all," "should it notify," and "should it assert presence" are three separate questions, not one. Cramming them into a single enum/flag produces combinations you can't express later (what if a plugin wants A+C but not B?). Give each axis its own mechanism.
+3. **Identify the independent axes.** A feature request that arrives as "option A and option B" is often two or three orthogonal concerns bundled together — e.g. "should this exist at all," "should it notify," and "should it assert presence" are three separate questions, not one. Cramming them into a single enum/flag produces combinations you can't express later (what if a plugin wants A+C but not B?). Give each axis its own mechanism. A recurring instance of this: if the design detects or handles data that indicates a likely security-relevant event (a plugin sanitizing suspicious/attacker-influenced input, a rejected/malformed value, an auth failure, etc.), "log it" and "alert the user about it" are two separate axes, not one. Server-log visibility (`mylog("none", ...)` in this codebase — the always-shown floor, matching how real errors are already logged) is close to a hard requirement whenever such an event is detected at all; a user-facing alert (`write_notification()`) is a materially bigger decision — it's persistent, unprompted, and (per existing precedent, e.g. `api_server_start.py`'s unauthorized-access alert) not rate-limited anywhere in this codebase, so a repeat offender can spam it. Don't let "should we detect/handle this" and "should we alert the user about it" collapse into one decision — record the logging as close to a given, and the user-facing alert as its own, explicit, separately-decidable (and separately deferrable) open issue.
 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 actually intended.
+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).
 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. **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.
 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.
+    - Did a correction made mid-document 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.
     - Does anything render incorrectly as markdown — an unfenced ASCII diagram or code block will collapse into one line under lazy-paragraph-continuation, the same class of bug as a list missing its preceding blank line.
-    - Do any internal anchor links' slugs actually match their headings?
+    - Do any internal anchor links' slugs match their headings?
     - Does the design still cleanly separate its axes, or did a later addition quietly re-conflate two concerns inside what's supposed to be a single-purpose mechanism (the same mistake step 3 exists to catch at the top level can reappear one level down inside an individual mechanism's own value set — e.g. a 3-value enum where two of the values are secretly independent booleans in a trenchcoat).
 11. **Leave a visible trail of corrections instead of silently rewriting.** When a review pass — yours or someone else's — finds something wrong, write "**Correction (caught in review):** ..." inline rather than quietly fixing the earlier text and moving on. This is what makes a PRD trustworthy to a second reader: they can see what was checked and what changed, not just receive a polished final answer with no visible seams.
+12. **When marking a PRD Implemented, ask once whether anything found along the way generalizes beyond this one document** — a tooling trick, a wrong assumption that took real digging to disprove, a review catch that would recur on the next PRD. The PRD's own "Implementation notes" section is the right place for what happened *in this PRD*; a skill or memory update is the right place for anything that would otherwise have to be rediscovered next time. One question, not a mandatory new section — skip it when nothing generalizes, don't force it on every PRD regardless of size.
 
 ## Structure to follow
 
@@ -45,7 +46,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both
 - **Open issues** — each with an explicit recorded decision (step 6), not left dangling.
 - **Affected files** — concrete `file:function:line` references, not bare filenames.
 - **Backward compatibility** — explicit default values and why they preserve current behavior for existing consumers.
-- **Performance impact** — the baseline (what's unindexed/slow *today*, independent of this change), what the change adds that's negligible, what's genuinely new and worth mitigating, and concrete mitigations rather than a vague "should be fine" (step 9).
+- **Performance impact** — the baseline (what's unindexed/slow *today*, independent of this change), what the change adds that's negligible, what's new and worth mitigating, and concrete mitigations rather than a vague "should be fine" (step 9).
 - **Docs/skills to update** — anywhere this needs to be reflected outside the code itself (external docs, paired skill files, template files new authors copy from).
 - **Tests** — organized by mechanism, each case naming the real function/query it exercises and the concrete assertion (step 7), plus a manual verification checklist for anything that can't be unit-tested (including an `EXPLAIN QUERY PLAN` check at realistic scale if the Performance impact section found a genuine risk).
 - **(Optional) Execution plan** — phased, referencing the same file/function names used above rather than restating the design in vaguer terms.
@@ -56,4 +57,4 @@ If a skill already documents the subsystem the feature touches, load it before r
 
 ## Where to save
 
-`.gemini/internal-docs/PRDs/<kebab-case-name>.md`, unless the user specifies otherwise. Mark the status line (`**Status:** Draft — pending review`) so it's clear this hasn't been approved yet, and keep the author line accurate about who actually made the calls (a design discussion with an assistant is not sole assistant authorship).
+`.gemini/internal-docs/PRDs/<kebab-case-name>.md`, unless the user specifies otherwise. Mark the status line (`**Status:** Draft — pending review`) so it's clear this hasn't been approved yet, and keep the author line accurate about who made the calls (a design discussion with an assistant is not sole assistant authorship).

.github/skills/scan-pipeline/SKILL.md

@@ -34,7 +34,7 @@ Covers what happens after a plugin's rows land in `CurrentScan`: presence comput
 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` — `update_ipv4_ipv6()` does **not** go through `LatestDeviceScan`/`FIELD_SPECS`; it reads `CurrentScan` directly with its own `PARTITION BY scanMac, address_family` ranking, so a device reporting both an IPv4 and an IPv6 row in the same cycle gets both `devPrimaryIPv4`/`devPrimaryIPv6` set from that cycle, not just whichever family happened to win `devLastIP`'s single-value reduction. See `.gemini/internal-docs/PRDs/dual-stack-primary-ip-support.md`.
+9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` — `update_ipv4_ipv6()` does **not** go through `LatestDeviceScan`/`FIELD_SPECS`; it reads `CurrentScan` directly with its own `PARTITION BY scanMac, address_family` ranking, so a device reporting both an IPv4 and an IPv6 row in the same cycle gets both `devPrimaryIPv4`/`devPrimaryIPv6` set from that cycle, not just whichever family happened to win `devLastIP`'s single-value reduction.
 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`. `Sessions` reflects step 10's pairing from here.
 12. `insertOnlineHistory(db)` — dashboard graph rollup.