From cc3ccbcaf0ab2d7bc09fbb207c725b1f61bfb8ff Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Thu, 10 Sep 2026 10:59:33 +1000 Subject: [PATCH] DOCS: skills + SQL cleanup --- .claude/skills/prd-writing/SKILL.md | 59 +++++++++++++++++++++++++ .claude/skills/scan-pipeline/SKILL.md | 1 + .gemini/skills/prd-writing/SKILL.md | 59 +++++++++++++++++++++++++ .gemini/skills/scan-pipeline/SKILL.md | 1 + .gemini/skills/skills-index/SKILL.md | 1 + .github/copilot-instructions.md | 1 + .github/skills/prd-writing/SKILL.md | 59 +++++++++++++++++++++++++ .github/skills/scan-pipeline/SKILL.md | 1 + .github/skills/skills-overview/SKILL.md | 1 + scripts/check_skill_pairs.py | 1 + server/db/db_upgrade.py | 8 ++++ server/db/schema/app.sql | 5 ++- 12 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/prd-writing/SKILL.md create mode 100644 .gemini/skills/prd-writing/SKILL.md create mode 100644 .github/skills/prd-writing/SKILL.md diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md new file mode 100644 index 00000000..d7b83631 --- /dev/null +++ b/.claude/skills/prd-writing/SKILL.md @@ -0,0 +1,59 @@ +--- +name: prd-writing +description: Read before writing a PRD, design doc, or feature proposal. Covers challenging the idea before designing it, verifying every claim against actual code (not memory or a plugin's name/category), tracing every downstream consumer of a new mechanism, evaluating performance impact against the schema/indexes that actually exist, recording rejected alternatives and open-issue decisions explicitly, and a dedicated final-check pass before calling it done. +--- + +# PRD Writing + +## When to use + +Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "create a PRD for X". Reserve this for changes where getting the design wrong is expensive to unwind — new cross-cutting mechanisms, schema changes, anything touching multiple subsystems. A one-file bug fix doesn't need this process. + +## 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: + +- 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. + +Both mistakes were plausible, well-written, and wrong. Neither would have survived actually reading the code first. + +## Process + +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. +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. +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. +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. +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. + - 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? + - 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. + +## Structure to follow + +- **Problem** — grounded in specific, cited current behavior, not a general complaint. +- **Goals / Non-goals** — non-goals should name specific things that sound in-scope but aren't, each with a one-line reason. +- **Design** — one subsection per independent axis/mechanism (step 3). Include a `### Rejected: X` subsection for any alternative seriously considered (step 5). +- **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). +- **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. + +## Before starting: check for an existing architecture-reference skill + +If a skill already documents the subsystem the feature touches, load it before researching from scratch — don't re-derive call graphs or mechanism details that are already written down. If the feature touches a subsystem with no such skill, and understanding it required significant re-derivation from raw code, that's a signal to write one afterward so the next PRD in that area doesn't start from zero. + +## 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). diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index b53ac7c7..ccd35d11 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -51,6 +51,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see the 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. 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 the otherwise-unused `server/db/schema/app.sql` reference copy) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Any *new* query added here should be checked the same way (`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. ## When to read this vs. other docs/skills diff --git a/.gemini/skills/prd-writing/SKILL.md b/.gemini/skills/prd-writing/SKILL.md new file mode 100644 index 00000000..6c90fc63 --- /dev/null +++ b/.gemini/skills/prd-writing/SKILL.md @@ -0,0 +1,59 @@ +--- +name: prd-writing +description: Rigorous PRD-writing methodology — challenge the idea, verify every claim against actual code, trace every downstream consumer of a mechanism, evaluate performance impact against the schema/indexes that actually exist, record rejected alternatives and open-issue decisions explicitly, and do a dedicated final-check pass before calling it done. Use this when asked to write, draft, or review a PRD, design doc, or feature proposal for this codebase. +--- + +# PRD Writing + +## When to use + +Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "create a PRD for X". Reserve this for changes where getting the design wrong is expensive to unwind — new cross-cutting mechanisms, schema changes, anything touching multiple subsystems. A one-file bug fix doesn't need this process. + +## 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: + +- 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. + +Both mistakes were plausible, well-written, and wrong. Neither would have survived actually reading the code first. + +## Process + +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. +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. +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. +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. +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. + - 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? + - 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. + +## Structure to follow + +- **Problem** — grounded in specific, cited current behavior, not a general complaint. +- **Goals / Non-goals** — non-goals should name specific things that sound in-scope but aren't, each with a one-line reason. +- **Design** — one subsection per independent axis/mechanism (step 3). Include a `### Rejected: X` subsection for any alternative seriously considered (step 5). +- **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). +- **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. + +## Before starting: check for an existing architecture-reference skill + +If a skill already documents the subsystem the feature touches, load it before researching from scratch — don't re-derive call graphs or mechanism details that are already written down. If the feature touches a subsystem with no such skill, and understanding it required significant re-derivation from raw code, that's a signal to write one afterward so the next PRD in that area doesn't start from zero. + +## 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). diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index 95189f09..64dc00d6 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -51,6 +51,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see the 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. 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 the otherwise-unused `server/db/schema/app.sql` reference copy) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Any *new* query added here should be checked the same way (`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. ## When to read this vs. other docs/skills diff --git a/.gemini/skills/skills-index/SKILL.md b/.gemini/skills/skills-index/SKILL.md index 5d25d739..396d2c83 100644 --- a/.gemini/skills/skills-index/SKILL.md +++ b/.gemini/skills/skills-index/SKILL.md @@ -30,6 +30,7 @@ Skills with the same purpose exist in more than one, sometimes under different n | Logging | `logging-standards` | `logging-standards` | — | `mylog` levels, message format, what not to log | | Scan pipeline internals | `scan-pipeline` | `scan-pipeline` | `scan-pipeline` | `process_scan()` call order and why it's load-bearing, `CurrentScan`/`Events`/`Sessions`/`DevicesView` relationships, how a session actually closes (no `close_session()` exists), and the `FIELD_SPECS` field-write authority mechanism. Complements `database-patterns` (Devices write-path/`*Source` attribution) rather than duplicating it. | | Database patterns | `database-patterns` | `database-patterns` | `database-patterns` | Devices table write-path inventory, the `FIELD_SOURCE_MAP`/`*Source` attribution system in `server/db/authoritative_handler.py`, SQLite trigger vs. Python-hook tradeoffs, and event-sourced vs. snapshot audit logging. | +| PRD writing | `prd-writing` | `prd-writing` | `prd-writing` | Methodology for writing a design doc: challenge the idea, verify every claim against actual code, trace every downstream consumer of a new mechanism, evaluate performance impact against the real schema/indexes, record rejected alternatives and open-issue decisions explicitly, final-check pass before done. Distilled from the `plugin-import-behavior-controls` PRD process, including real mistakes caught mid-review. | --- diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ea5fc325..2bbf60d5 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -46,6 +46,7 @@ Procedural knowledge lives in `.github/skills/`. Load the appropriate skill when | Coding standards | `code-standards` | | Devices table write paths, SQLite triggers, audit logging, `*Source` attribution | `database-patterns` | | `process_scan()` call order, CurrentScan/Events/Sessions internals, device presence lifecycle | `scan-pipeline` | +| Write a PRD, design doc, or feature proposal | `prd-writing` | ## Execution Protocol diff --git a/.github/skills/prd-writing/SKILL.md b/.github/skills/prd-writing/SKILL.md new file mode 100644 index 00000000..eedabe24 --- /dev/null +++ b/.github/skills/prd-writing/SKILL.md @@ -0,0 +1,59 @@ +--- +name: netalertx-prd-writing +description: Rigorous PRD-writing methodology for NetAlertX — challenge the idea, verify every claim against actual code, trace every downstream consumer of a mechanism, evaluate performance impact against the schema/indexes that actually exist, record rejected alternatives and open-issue decisions explicitly, and do a dedicated final-check pass before calling it done. Use this when asked to write, draft, or review a PRD, design doc, or feature proposal. +--- + +# PRD Writing + +## When to use + +Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "create a PRD for X". Reserve this for changes where getting the design wrong is expensive to unwind — new cross-cutting mechanisms, schema changes, anything touching multiple subsystems. A one-file bug fix doesn't need this process. + +## 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: + +- 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. + +Both mistakes were plausible, well-written, and wrong. Neither would have survived actually reading the code first. + +## Process + +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. +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. +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. +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. +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. + - 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? + - 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. + +## Structure to follow + +- **Problem** — grounded in specific, cited current behavior, not a general complaint. +- **Goals / Non-goals** — non-goals should name specific things that sound in-scope but aren't, each with a one-line reason. +- **Design** — one subsection per independent axis/mechanism (step 3). Include a `### Rejected: X` subsection for any alternative seriously considered (step 5). +- **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). +- **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. + +## Before starting: check for an existing architecture-reference skill + +If a skill already documents the subsystem the feature touches, load it before researching from scratch — don't re-derive call graphs or mechanism details that are already written down. If the feature touches a subsystem with no such skill, and understanding it required significant re-derivation from raw code, that's a signal to write one afterward so the next PRD in that area doesn't start from zero. + +## 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). diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 4e6bd057..670a9acf 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -51,6 +51,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see the 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. 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 the otherwise-unused `server/db/schema/app.sql` reference copy) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Any *new* query added here should be checked the same way (`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. ## When to read this vs. other docs/skills diff --git a/.github/skills/skills-overview/SKILL.md b/.github/skills/skills-overview/SKILL.md index 5afe10f7..19e206e8 100644 --- a/.github/skills/skills-overview/SKILL.md +++ b/.github/skills/skills-overview/SKILL.md @@ -30,6 +30,7 @@ Skills with the same purpose exist in more than one, sometimes under different n | Logging | `logging-standards` | `logging-standards` | — | `mylog` levels, message format, what not to log | | Scan pipeline internals | `scan-pipeline` | `scan-pipeline` | `scan-pipeline` | `process_scan()` call order and why it's load-bearing, `CurrentScan`/`Events`/`Sessions`/`DevicesView` relationships, how a session actually closes (no `close_session()` exists), and the `FIELD_SPECS` field-write authority mechanism. Complements `database-patterns` (Devices write-path/`*Source` attribution) rather than duplicating it. | | Database patterns | `database-patterns` | `database-patterns` | `database-patterns` | Devices table write-path inventory, the `FIELD_SOURCE_MAP`/`*Source` attribution system in `server/db/authoritative_handler.py`, SQLite trigger vs. Python-hook tradeoffs, and event-sourced vs. snapshot audit logging. | +| PRD writing | `prd-writing` | `prd-writing` | `prd-writing` | Methodology for writing a design doc: challenge the idea, verify every claim against actual code, trace every downstream consumer of a new mechanism, evaluate performance impact against the real schema/indexes, record rejected alternatives and open-issue decisions explicitly, final-check pass before done. Distilled from the `plugin-import-behavior-controls` PRD process, including real mistakes caught mid-review. | --- diff --git a/scripts/check_skill_pairs.py b/scripts/check_skill_pairs.py index 6cc4ed04..91825603 100644 --- a/scripts/check_skill_pairs.py +++ b/scripts/check_skill_pairs.py @@ -30,6 +30,7 @@ GROUPS = [ [".gemini/skills/pr-analysis/SKILL.md", ".github/skills/pr-analysis/SKILL.md", ".claude/skills/pr-analysis/SKILL.md"], [".gemini/skills/scan-pipeline/SKILL.md", ".github/skills/scan-pipeline/SKILL.md", ".claude/skills/scan-pipeline/SKILL.md"], [".gemini/skills/database-patterns/SKILL.md", ".github/skills/database-patterns/SKILL.md", ".claude/skills/database-patterns/SKILL.md"], + [".gemini/skills/prd-writing/SKILL.md", ".github/skills/prd-writing/SKILL.md", ".claude/skills/prd-writing/SKILL.md"], [".gemini/skills/settings/SKILL.md", ".github/skills/settings-management/SKILL.md"], [".gemini/skills/mcp-activation/SKILL.md", ".github/skills/mcp-activation/SKILL.md"], [".gemini/skills/project-navigation/SKILL.md", ".github/skills/project-navigation/SKILL.md"], diff --git a/server/db/db_upgrade.py b/server/db/db_upgrade.py index f2e9b55f..3851f8e8 100755 --- a/server/db/db_upgrade.py +++ b/server/db/db_upgrade.py @@ -575,6 +575,14 @@ def ensure_CurrentScan(sql) -> bool: scanType STRING(250) ); """) + # scanMac has no uniqueness constraint - multiple plugins commonly report + # the same MAC in one cycle (e.g. arp_scan + nslookup), so every lookup + # keyed on scanMac (update_presence_from_CurrentScan, insert_events, the + # LatestDeviceScan/LatestEventsPerMAC views) was a full table scan without + # this. Table is dropped every cycle, so the index is rebuilt with it - + # cheap insurance against O(n^2) scans at NOC-scale device counts (10k+ in + # real deployments). + sql.execute("CREATE INDEX IF NOT EXISTS idx_currentscan_scanmac ON CurrentScan(scanMac);") return True diff --git a/server/db/schema/app.sql b/server/db/schema/app.sql index dffd27b1..66358d01 100644 --- a/server/db/schema/app.sql +++ b/server/db/schema/app.sql @@ -164,9 +164,10 @@ CREATE TABLE CurrentScan ( scanVlan STRING(250), scanParentMAC STRING(250), scanParentPort STRING(250), - scanType STRING(250), - UNIQUE(scanMac) + scanFQDN STRING(250), + scanType STRING(250) ); +CREATE INDEX idx_currentscan_scanmac ON CurrentScan(scanMac); CREATE TABLE IF NOT EXISTS AppEvents ( "index" INTEGER PRIMARY KEY AUTOINCREMENT, guid TEXT UNIQUE,