From 1c410bf2bef836b6538876f4e85547ce7a7afa6f Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Thu, 24 Sep 2026 09:00:08 +1000 Subject: [PATCH] DOCS: skill updates --- .claude/skills/plugin-review/SKILL.md | 16 +++++++++++++--- .claude/skills/prd-writing/SKILL.md | 13 +++++++------ .claude/skills/scan-pipeline/SKILL.md | 2 +- .gemini/skills/plugin-review/SKILL.md | 16 +++++++++++++--- .gemini/skills/prd-writing/SKILL.md | 13 +++++++------ .gemini/skills/scan-pipeline/SKILL.md | 2 +- .github/skills/plugin-review/SKILL.md | 16 +++++++++++++--- .github/skills/prd-writing/SKILL.md | 13 +++++++------ .github/skills/scan-pipeline/SKILL.md | 2 +- 9 files changed, 63 insertions(+), 30 deletions(-) diff --git a/.claude/skills/plugin-review/SKILL.md b/.claude/skills/plugin-review/SKILL.md index 64c2a02e4..b7f4a3f10 100644 --- a/.claude/skills/plugin-review/SKILL.md +++ b/.claude/skills/plugin-review/SKILL.md @@ -1,6 +1,6 @@ --- name: 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. diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md index 523f4764e..9c5cc388f 100644 --- a/.claude/skills/prd-writing/SKILL.md +++ b/.claude/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/.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/.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). diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 9cbd4a7e0..4d37074e9 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/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. diff --git a/.gemini/skills/plugin-review/SKILL.md b/.gemini/skills/plugin-review/SKILL.md index 64c2a02e4..b7f4a3f10 100644 --- a/.gemini/skills/plugin-review/SKILL.md +++ b/.gemini/skills/plugin-review/SKILL.md @@ -1,6 +1,6 @@ --- name: 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. diff --git a/.gemini/skills/prd-writing/SKILL.md b/.gemini/skills/prd-writing/SKILL.md index 5121ecd7f..6c412894b 100644 --- a/.gemini/skills/prd-writing/SKILL.md +++ b/.gemini/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/.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/.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). diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index 0a5be06d9..40b1c181f 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/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. diff --git a/.github/skills/plugin-review/SKILL.md b/.github/skills/plugin-review/SKILL.md index e10927e8e..4cdcf1f4e 100644 --- a/.github/skills/plugin-review/SKILL.md +++ b/.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. diff --git a/.github/skills/prd-writing/SKILL.md b/.github/skills/prd-writing/SKILL.md index e5766a058..f64e496ce 100644 --- a/.github/skills/prd-writing/SKILL.md +++ b/.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/.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/.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). diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 89267fc68..f1d086ad3 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.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.