diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md index 3ca701abc..523f4764e 100644 --- a/.claude/skills/prd-writing/SKILL.md +++ b/.claude/skills/prd-writing/SKILL.md @@ -20,7 +20,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both ## 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. +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. 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. @@ -28,7 +28,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 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. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` — don't assume a column is indexed just because it looks like a key (check `server/db/db_upgrade.py`/`server/db/schema/app.sql`). Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming either way; if it comes back unindexed, a `GROUP BY` aggregate is the usual fix. +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. - Does every "this is the cleanest/simplest real case" claim still hold up if you actually re-read that specific piece of code right now, or was it asserted by pattern-matching a name/category? Re-verify, don't re-assert. diff --git a/.claude/skills/skill-hygiene/SKILL.md b/.claude/skills/skill-hygiene/SKILL.md index 7befeabe5..240bcaeb4 100644 --- a/.claude/skills/skill-hygiene/SKILL.md +++ b/.claude/skills/skill-hygiene/SKILL.md @@ -1,6 +1,6 @@ --- name: skill-hygiene -description: Read before writing or editing any SKILL.md in this repo (.claude/.gemini/.github skills trees). Covers the two standing rules for skill prose - state current behavior only, and prefer plain, short wording - plus the grep sweep to run before calling a skill clean. +description: Read before writing or editing any SKILL.md, or any research/audit doc in .gemini/internal-docs/research/. Covers the two standing rules for living-reference prose - state current behavior only, and prefer plain, short wording - plus the grep sweep to run before calling a doc clean. PRDs are the deliberate exception (they keep a correction trail). --- # Skill Hygiene @@ -39,6 +39,17 @@ grep -rniE "as of 202|caught in review|caught mid-review|correction:|correction Read every hit in context — some are legitimate (a rule instructing PRD authors to write correction trails, or "previously down" describing device state, are not violations). Fix the ones that narrate the skill's own history instead of the system's current behavior. +## Also applies to: research/audit docs + +The same two rules apply to `.gemini/internal-docs/research/*.md` (architecture audit docs) - they're a live reference for the system's current known issues, not a changelog of what's been fixed. When a finding is resolved: + +- Remove it from the live doc entirely. Don't leave a struck-through "Fixed 2026-09-14" row — a resolved item isn't a current priority, and tracking it that way is clutter against the doc's actual point (what to work on next). +- If the original diagnosis has real archival value (the reasoning, what was ruled out, exact figures), copy the relevant section into `.gemini/internal-docs/research_old/` before deleting it from the live doc, rather than losing it outright. +- If it doesn't (a one-line finding with an obvious, already-applied fix), just delete it — no archive needed. +- Research docs don't need cross-tree sync the way skills do (they only live in `.gemini/internal-docs/research/`), so this section doesn't apply to them. + +This does not apply to PRDs (`.gemini/internal-docs/PRDs/`) — those keep their correction trail deliberately, per `prd-writing`. + ## Keep the three trees in sync Most skills exist as three near-identical copies (`.claude/skills//SKILL.md`, `.gemini/skills//SKILL.md`, `.github/skills//SKILL.md` — see `.gemini/skills/skills-index/SKILL.md` for the pairing map). When a hygiene fix changes a skill's body, apply the same fix to all paired copies so they stay identical (frontmatter `name`/`description` may differ per tree's own convention; the body should not). `scripts/check_skill_pairs.py`'s `GROUPS` list only flags when some-but-not-all paired files changed in a diff — it doesn't check the bodies actually match, so a manual diff after editing is still worth it. diff --git a/.gemini/skills/prd-writing/SKILL.md b/.gemini/skills/prd-writing/SKILL.md index ed7e6adaf..5121ecd7f 100644 --- a/.gemini/skills/prd-writing/SKILL.md +++ b/.gemini/skills/prd-writing/SKILL.md @@ -20,7 +20,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both ## 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. +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. 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. @@ -28,7 +28,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 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. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` — don't assume a column is indexed just because it looks like a key (check `server/db/db_upgrade.py`/`server/db/schema/app.sql`). Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming either way; if it comes back unindexed, a `GROUP BY` aggregate is the usual fix. +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. - Does every "this is the cleanest/simplest real case" claim still hold up if you actually re-read that specific piece of code right now, or was it asserted by pattern-matching a name/category? Re-verify, don't re-assert. diff --git a/.gemini/skills/skill-hygiene/SKILL.md b/.gemini/skills/skill-hygiene/SKILL.md index 7befeabe5..240bcaeb4 100644 --- a/.gemini/skills/skill-hygiene/SKILL.md +++ b/.gemini/skills/skill-hygiene/SKILL.md @@ -1,6 +1,6 @@ --- name: skill-hygiene -description: Read before writing or editing any SKILL.md in this repo (.claude/.gemini/.github skills trees). Covers the two standing rules for skill prose - state current behavior only, and prefer plain, short wording - plus the grep sweep to run before calling a skill clean. +description: Read before writing or editing any SKILL.md, or any research/audit doc in .gemini/internal-docs/research/. Covers the two standing rules for living-reference prose - state current behavior only, and prefer plain, short wording - plus the grep sweep to run before calling a doc clean. PRDs are the deliberate exception (they keep a correction trail). --- # Skill Hygiene @@ -39,6 +39,17 @@ grep -rniE "as of 202|caught in review|caught mid-review|correction:|correction Read every hit in context — some are legitimate (a rule instructing PRD authors to write correction trails, or "previously down" describing device state, are not violations). Fix the ones that narrate the skill's own history instead of the system's current behavior. +## Also applies to: research/audit docs + +The same two rules apply to `.gemini/internal-docs/research/*.md` (architecture audit docs) - they're a live reference for the system's current known issues, not a changelog of what's been fixed. When a finding is resolved: + +- Remove it from the live doc entirely. Don't leave a struck-through "Fixed 2026-09-14" row — a resolved item isn't a current priority, and tracking it that way is clutter against the doc's actual point (what to work on next). +- If the original diagnosis has real archival value (the reasoning, what was ruled out, exact figures), copy the relevant section into `.gemini/internal-docs/research_old/` before deleting it from the live doc, rather than losing it outright. +- If it doesn't (a one-line finding with an obvious, already-applied fix), just delete it — no archive needed. +- Research docs don't need cross-tree sync the way skills do (they only live in `.gemini/internal-docs/research/`), so this section doesn't apply to them. + +This does not apply to PRDs (`.gemini/internal-docs/PRDs/`) — those keep their correction trail deliberately, per `prd-writing`. + ## Keep the three trees in sync Most skills exist as three near-identical copies (`.claude/skills//SKILL.md`, `.gemini/skills//SKILL.md`, `.github/skills//SKILL.md` — see `.gemini/skills/skills-index/SKILL.md` for the pairing map). When a hygiene fix changes a skill's body, apply the same fix to all paired copies so they stay identical (frontmatter `name`/`description` may differ per tree's own convention; the body should not). `scripts/check_skill_pairs.py`'s `GROUPS` list only flags when some-but-not-all paired files changed in a diff — it doesn't check the bodies actually match, so a manual diff after editing is still worth it. diff --git a/.gemini/skills/skills-index/SKILL.md b/.gemini/skills/skills-index/SKILL.md index 3cd274d7d..bd47c8b52 100644 --- a/.gemini/skills/skills-index/SKILL.md +++ b/.gemini/skills/skills-index/SKILL.md @@ -32,7 +32,7 @@ Skills with the same purpose exist in more than one, sometimes under different n | 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. | | UX/frontend design | `ux-design-patterns` | `ux-design-patterns` | `ux-design-patterns` | Don't invent new UX behavior/visual patterns unless a PRD calls for it - search `front/` for an existing pattern first and reuse it. Priority order for design tradeoffs when several options are reasonable: existing behavior > intuitiveness > information density > usability > utility > uniqueness > industry practices > generic UI. | -| Skill hygiene | `skill-hygiene` | `skill-hygiene` | `skill-hygiene` | Read before writing/editing any SKILL.md. Two standing rules: state current behavior only (no "Correction:", no "as of ", no "caught in review" narration - that trail belongs in PRDs), and prefer plain, short wording. Includes the grep sweep to run before calling a skill clean. | +| Skill hygiene | `skill-hygiene` | `skill-hygiene` | `skill-hygiene` | Read before writing/editing any SKILL.md, or any research/audit doc in `.gemini/internal-docs/research/`. Two standing rules: state current behavior only (no "Correction:", no "as of ", no "caught in review" narration - that trail belongs in PRDs), and prefer plain, short wording. Includes the grep sweep to run before calling a doc clean. | --- diff --git a/.github/skills/prd-writing/SKILL.md b/.github/skills/prd-writing/SKILL.md index 807248aa9..e5766a058 100644 --- a/.github/skills/prd-writing/SKILL.md +++ b/.github/skills/prd-writing/SKILL.md @@ -20,7 +20,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both ## 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. +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. 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. @@ -28,7 +28,7 @@ Both are plausible, well-written, and wrong. Reading the code first catches both 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. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` — don't assume a column is indexed just because it looks like a key (check `server/db/db_upgrade.py`/`server/db/schema/app.sql`). Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming either way; if it comes back unindexed, a `GROUP BY` aggregate is the usual fix. +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. - Does every "this is the cleanest/simplest real case" claim still hold up if you actually re-read that specific piece of code right now, or was it asserted by pattern-matching a name/category? Re-verify, don't re-assert. diff --git a/.github/skills/skill-hygiene/SKILL.md b/.github/skills/skill-hygiene/SKILL.md index 379dc3fea..609a733ec 100644 --- a/.github/skills/skill-hygiene/SKILL.md +++ b/.github/skills/skill-hygiene/SKILL.md @@ -1,6 +1,6 @@ --- name: netalertx-skill-hygiene -description: Read before writing or editing any SKILL.md in this repo (.claude/.gemini/.github skills trees). Covers the two standing rules for skill prose - state current behavior only, and prefer plain, short wording - plus the grep sweep to run before calling a skill clean. +description: Read before writing or editing any SKILL.md, or any research/audit doc in .gemini/internal-docs/research/. Covers the two standing rules for living-reference prose - state current behavior only, and prefer plain, short wording - plus the grep sweep to run before calling a doc clean. PRDs are the deliberate exception (they keep a correction trail). --- # Skill Hygiene @@ -39,6 +39,17 @@ grep -rniE "as of 202|caught in review|caught mid-review|correction:|correction Read every hit in context — some are legitimate (a rule instructing PRD authors to write correction trails, or "previously down" describing device state, are not violations). Fix the ones that narrate the skill's own history instead of the system's current behavior. +## Also applies to: research/audit docs + +The same two rules apply to `.gemini/internal-docs/research/*.md` (architecture audit docs) - they're a live reference for the system's current known issues, not a changelog of what's been fixed. When a finding is resolved: + +- Remove it from the live doc entirely. Don't leave a struck-through "Fixed 2026-09-14" row — a resolved item isn't a current priority, and tracking it that way is clutter against the doc's actual point (what to work on next). +- If the original diagnosis has real archival value (the reasoning, what was ruled out, exact figures), copy the relevant section into `.gemini/internal-docs/research_old/` before deleting it from the live doc, rather than losing it outright. +- If it doesn't (a one-line finding with an obvious, already-applied fix), just delete it — no archive needed. +- Research docs don't need cross-tree sync the way skills do (they only live in `.gemini/internal-docs/research/`), so this section doesn't apply to them. + +This does not apply to PRDs (`.gemini/internal-docs/PRDs/`) — those keep their correction trail deliberately, per `prd-writing`. + ## Keep the three trees in sync Most skills exist as three near-identical copies (`.claude/skills//SKILL.md`, `.gemini/skills//SKILL.md`, `.github/skills//SKILL.md` — see `.gemini/skills/skills-index/SKILL.md` for the pairing map). When a hygiene fix changes a skill's body, apply the same fix to all paired copies so they stay identical (frontmatter `name`/`description` may differ per tree's own convention; the body should not). `scripts/check_skill_pairs.py`'s `GROUPS` list only flags when some-but-not-all paired files changed in a diff — it doesn't check the bodies actually match, so a manual diff after editing is still worth it. diff --git a/.github/skills/skills-overview/SKILL.md b/.github/skills/skills-overview/SKILL.md index 84e4bc5fb..9a94b2931 100644 --- a/.github/skills/skills-overview/SKILL.md +++ b/.github/skills/skills-overview/SKILL.md @@ -32,7 +32,7 @@ Skills with the same purpose exist in more than one, sometimes under different n | 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. | | UX/frontend design | `ux-design-patterns` | `ux-design-patterns` | `ux-design-patterns` | Don't invent new UX behavior/visual patterns unless a PRD calls for it - search `front/` for an existing pattern first and reuse it. Priority order for design tradeoffs when several options are reasonable: existing behavior > intuitiveness > information density > usability > utility > uniqueness > industry practices > generic UI. | -| Skill hygiene | `skill-hygiene` | `skill-hygiene` | `skill-hygiene` | Read before writing/editing any SKILL.md. Two standing rules: state current behavior only (no "Correction:", no "as of ", no "caught in review" narration - that trail belongs in PRDs), and prefer plain, short wording. Includes the grep sweep to run before calling a skill clean. | +| Skill hygiene | `skill-hygiene` | `skill-hygiene` | `skill-hygiene` | Read before writing/editing any SKILL.md, or any research/audit doc in `.gemini/internal-docs/research/`. Two standing rules: state current behavior only (no "Correction:", no "as of ", no "caught in review" narration - that trail belongs in PRDs), and prefer plain, short wording. Includes the grep sweep to run before calling a doc clean. | --- diff --git a/.gitignore b/.gitignore index 1eec741f7..3439c08c5 100755 --- a/.gitignore +++ b/.gitignore @@ -29,6 +29,8 @@ front/log/* !.gemini/internal-docs/PRDs/to_review/.gitkeep .gemini/internal-docs/research/* !.gemini/internal-docs/research/.gitkeep +.gemini/internal-docs/research_old/* +!.gemini/internal-docs/research_old/.gitkeep /log/plugins/* front/api/* /api/* diff --git a/docs/PLUGINS_DEV_QUICK_START.md b/docs/PLUGINS_DEV_QUICK_START.md index 1998e31c5..395bbf878 100644 --- a/docs/PLUGINS_DEV_QUICK_START.md +++ b/docs/PLUGINS_DEV_QUICK_START.md @@ -17,6 +17,7 @@ Start from the template to get the basic structure: cd /workspaces/NetAlertX/server/plugins cp -r __template my_plugin cd my_plugin +mv rename_me.py script.py ``` ### 2. Update `config.json` Identifiers diff --git a/server/api_server/api_server_start.py b/server/api_server/api_server_start.py index 7193d1a3e..d55dfc53e 100755 --- a/server/api_server/api_server_start.py +++ b/server/api_server/api_server_start.py @@ -73,6 +73,7 @@ from .openapi.schemas import ( # noqa: E402 [flake8 lint suppression] DeviceSearchRequest, DeviceSearchResponse, DeviceListRequest, DeviceListResponse, + DeviceListAllRequest, DeviceListWrapperResponse, DeviceExportResponse, DeviceUpdateRequest, @@ -101,6 +102,7 @@ DbQueryUpdateRequest, DbQueryDeleteRequest, AddToQueueRequest, GetSettingResponse, RecentEventsRequest, SetDeviceAliasRequest, + EventListRequest, LanguagesResponse, PluginStatsResponse, ) @@ -636,14 +638,30 @@ def api_device_open_ports(payload=None): @validate_request( operation_id="get_all_devices", summary="Get All Devices", - description="Retrieve a list of all devices in the system. Returns all records. No pagination supported.", + description="Retrieve a list of all devices in the system, ordered by devMac. Returns every device if limit is omitted.", + request_model=DeviceListAllRequest, response_model=DeviceListWrapperResponse, + query_params=[{ + "name": "limit", + "in": "query", + "required": False, + "description": "Max devices to return", + "schema": {"type": "integer", "minimum": 1, "maximum": 1000} + }, { + "name": "offset", + "in": "query", + "required": False, + "description": "Number of devices to skip", + "schema": {"type": "integer", "minimum": 0} + }], tags=["devices"], auth_callable=is_authorized ) -def api_get_devices(payload=None): +def api_get_devices(payload: DeviceListAllRequest = None): + limit = payload.limit if payload else request.args.get("limit", type=int) + offset = payload.offset if payload else request.args.get("offset", type=int) device_handler = DeviceInstance() - devices = device_handler.getAll_AsResponse() + devices = device_handler.getAll_AsResponse(limit, offset) return jsonify({"success": True, "devices": devices}) @@ -1543,22 +1561,37 @@ def api_delete_all_events(payload=None): @validate_request( operation_id="get_all_events", summary="Get Events", - description="Retrieve a list of events, optionally filtered by MAC. Returns all matching records. No pagination supported.", + description="Retrieve a list of events, optionally filtered by MAC, ordered by eveDateTime descending. Returns every matching record if limit is omitted.", + request_model=EventListRequest, query_params=[{ "name": "mac", "description": "Filter by Device MAC", "required": False, "schema": {"type": "string"} + }, { + "name": "limit", + "in": "query", + "required": False, + "description": "Max events to return", + "schema": {"type": "integer", "minimum": 1, "maximum": 1000} + }, { + "name": "offset", + "in": "query", + "required": False, + "description": "Number of events to skip", + "schema": {"type": "integer", "minimum": 0} }], response_model=BaseResponse, tags=["events"], auth_callable=is_authorized ) -def api_get_events(payload=None): +def api_get_events(payload: EventListRequest = None): try: - mac = request.args.get("mac") + mac = payload.mac if payload else request.args.get("mac") + limit = payload.limit if payload else request.args.get("limit", type=int) + offset = payload.offset if payload else request.args.get("offset", type=int) event_handler = EventInstance() - events = event_handler.getEvents(mac) + events = event_handler.getEvents(mac, limit, offset) return jsonify({"success": True, "count": len(events), "events": events}) except (ValueError, RuntimeError) as e: mylog("verbose", [f"[api_get_events] Error: {e}"]) diff --git a/server/api_server/openapi/schemas.py b/server/api_server/openapi/schemas.py index 20a9a1fc0..a4ac30b76 100644 --- a/server/api_server/openapi/schemas.py +++ b/server/api_server/openapi/schemas.py @@ -245,6 +245,12 @@ class DeviceSearchResponse(BaseResponse): devices: List[DeviceInfo] = Field(default_factory=list, description="List of matching devices") +class DeviceListAllRequest(BaseModel): + """Request for listing all devices, with optional pagination.""" + limit: Optional[int] = Field(None, ge=1, le=1000, description="Max devices to return") + offset: Optional[int] = Field(None, ge=0, description="Number of devices to skip") + + class DeviceListRequest(BaseModel): """Request for listing devices by status.""" status: Optional[Literal[ @@ -753,6 +759,13 @@ class EventInfo(BaseModel): evePreviousIP: Optional[str] = Field(None, description="Previous IP if changed") +class EventListRequest(BaseModel): + """Request for listing events, optionally filtered by MAC, with optional pagination.""" + mac: Optional[str] = Field(None, description="Filter by Device MAC") + limit: Optional[int] = Field(None, ge=1, le=1000, description="Max events to return") + offset: Optional[int] = Field(None, ge=0, description="Number of events to skip") + + class RecentEventsRequest(BaseModel): """Request for recent events.""" hours: int = Field( diff --git a/server/db/db_helper.py b/server/db/db_helper.py index 16e172e01..ff85519e8 100755 --- a/server/db/db_helper.py +++ b/server/db/db_helper.py @@ -68,11 +68,18 @@ def get_device_condition_by_status(device_status): # ------------------------------------------------------------------------------- def get_sql_devices_tiles(): - """Build the device tiles count SQL using get_device_conditions() to avoid duplicating filter logic.""" + """Build the device tiles count SQL using get_device_conditions() to avoid duplicating filter logic. + + Single pass over DevicesView with conditional-aggregation SUM(CASE...) columns, + rather than one independent `(SELECT COUNT(*) FROM DevicesView WHERE ...)` + scalar subquery per tile - SQLite doesn't share view-materialization across + independent subqueries in one statement, so the previous shape evaluated + DevicesView's own per-row cost (including the devFlapping correlated + subquery) once per tile (9 times) instead of once total.""" conds = get_device_conditions() def f(key): - """Strip 'WHERE ' prefix for use inside SELECT subqueries.""" + """Strip 'WHERE ' prefix for use inside a CASE WHEN condition.""" return conds[key][len("WHERE "):] # UI_MY_DEVICES setting values mapped to their device_conditions keys @@ -85,33 +92,31 @@ def f(key): ] my_devices_clauses = "\n OR ".join( - f"(instr((SELECT setValue FROM Statuses), '{sk}') > 0 AND {f(ck)})" + f"(instr(Statuses.setValue, '{sk}') > 0 AND {f(ck)})" for sk, ck in my_devices_setting_map ) + def tile(key, label): + return f'COALESCE(SUM(CASE WHEN {f(key)} THEN 1 ELSE 0 END), 0) AS "{label}"' + return f""" WITH Statuses AS ( SELECT setValue FROM Settings WHERE setKey = 'UI_MY_DEVICES' - ), - MyDevicesFilter AS ( - SELECT devMac, devIsSleeping - FROM DevicesView - WHERE - {my_devices_clauses} ) SELECT - (SELECT COUNT(*) FROM DevicesView WHERE {f('connected')}) AS connected, - (SELECT COUNT(*) FROM DevicesView WHERE {f('offline')}) AS offline, - (SELECT COUNT(*) FROM DevicesView WHERE {f('down')}) AS down, - (SELECT COUNT(*) FROM DevicesView WHERE {f('new')}) AS new, - (SELECT COUNT(*) FROM DevicesView WHERE {f('archived')}) AS archived, - (SELECT COUNT(*) FROM DevicesView WHERE {f('favorites')}) AS favorites, - (SELECT COUNT(*) FROM DevicesView WHERE {f('all')}) AS "all", - (SELECT COUNT(*) FROM DevicesView) AS "all_devices", - (SELECT COUNT(*) FROM MyDevicesFilter) AS my_devices - FROM Statuses; + {tile('connected', 'connected')}, + {tile('offline', 'offline')}, + {tile('down', 'down')}, + {tile('new', 'new')}, + {tile('archived', 'archived')}, + {tile('favorites', 'favorites')}, + {tile('all', 'all')}, + COUNT(*) AS "all_devices", + COALESCE(SUM(CASE WHEN {my_devices_clauses} + THEN 1 ELSE 0 END), 0) AS "my_devices" + FROM DevicesView, Statuses; """ diff --git a/server/db/db_upgrade.py b/server/db/db_upgrade.py index 86cbb8dc6..4de9357b6 100755 --- a/server/db/db_upgrade.py +++ b/server/db/db_upgrade.py @@ -578,6 +578,14 @@ def ensure_Indexes(sql) -> bool: ON Devices(LOWER(devParentMAC)) """, ), + ( + "idx_dev_guid", + "CREATE INDEX idx_dev_guid ON Devices(devGUID)", + ), + ( + "idx_plobj_guid", + "CREATE INDEX idx_plobj_guid ON Plugins_Objects(objectGuid)", + ), # Optional filter indexes ("idx_dev_site", "CREATE INDEX idx_dev_site ON Devices(devSite)"), ("idx_dev_group", "CREATE INDEX idx_dev_group ON Devices(devGroup)"), diff --git a/server/db/schema/app.sql b/server/db/schema/app.sql index b78d652a9..bdbb55e3b 100644 --- a/server/db/schema/app.sql +++ b/server/db/schema/app.sql @@ -224,6 +224,8 @@ CREATE INDEX IDX_dev_Favorite ON Devices (devFavorite); CREATE INDEX IDX_dev_LastIP ON Devices (devLastIP); CREATE INDEX IDX_dev_NewDevice ON Devices (devIsNew); CREATE INDEX IDX_dev_Archived ON Devices (devIsArchived); +CREATE INDEX idx_dev_guid ON Devices(devGUID); +CREATE INDEX idx_plobj_guid ON Plugins_Objects(objectGuid); CREATE UNIQUE INDEX IF NOT EXISTS idx_events_unique ON Events ( eveMac, diff --git a/server/models/device_instance.py b/server/models/device_instance.py index 50e7e9061..d070868c9 100755 --- a/server/models/device_instance.py +++ b/server/models/device_instance.py @@ -46,8 +46,20 @@ def _execute(self, query, params=()): conn.close() # --- public API ----------------------------------------------------------- - def getAll(self): - return self._fetchall("SELECT * FROM Devices") + def getAll(self, limit=None, offset=None): + """Return all devices, ordered by devMac for stable pagination. + Returns every device if limit is omitted.""" + query = "SELECT * FROM Devices ORDER BY devMac" + params = [] + if limit is not None: + query += " LIMIT ? OFFSET ?" + params.extend([limit, offset or 0]) + elif offset is not None: + # SQLite's unlimited-limit form - an offset with no limit still + # needs a LIMIT clause for OFFSET to take effect. + query += " LIMIT -1 OFFSET ?" + params.append(offset) + return self._fetchall(query, params) def getUnknown(self): return self._fetchall(""" @@ -229,9 +241,9 @@ def getOpenPorts(self, target): # --- devices_endpoint.py methods (HTTP response layer) ------------------- - def getAll_AsResponse(self): + def getAll_AsResponse(self, limit=None, offset=None): """Return all devices as raw data (not jsonified).""" - return self.getAll() + return self.getAll(limit, offset) def deleteDevices(self, macs): """ diff --git a/server/models/event_instance.py b/server/models/event_instance.py index 6222fd3bd..ff1e82151 100644 --- a/server/models/event_instance.py +++ b/server/models/event_instance.py @@ -136,21 +136,34 @@ def createEvent(self, mac: str, ip: str, event_type: str = "Device Down", additi mylog("debug", f"[Events] Created event for {mac} ({event_type})") return {"success": True, "message": f"Created event for {mac}"} - def getEvents(self, mac=None): + def getEvents(self, mac=None, limit=None, offset=None): """ - Fetch all events, or events for a specific MAC if provided. - Returns list of events. + Fetch all events, or events for a specific MAC if provided, ordered by + eveDateTime descending. Returns every matching event if limit is omitted. """ conn = self._conn() cur = conn.cursor() + # rowid DESC is a tiebreaker for events sharing the same eveDateTime + # (only second precision) - without it, LIMIT/OFFSET pages aren't + # guaranteed to reconstruct the same order as the unpaginated query. if mac: - sql = "SELECT * FROM Events WHERE eveMac=? ORDER BY eveDateTime DESC" - cur.execute(sql, (mac,)) + sql = "SELECT * FROM Events WHERE eveMac=? ORDER BY eveDateTime DESC, rowid DESC" + params = [mac] else: - sql = "SELECT * FROM Events ORDER BY eveDateTime DESC" - cur.execute(sql) - + sql = "SELECT * FROM Events ORDER BY eveDateTime DESC, rowid DESC" + params = [] + + if limit is not None: + sql += " LIMIT ? OFFSET ?" + params.extend([limit, offset or 0]) + elif offset is not None: + # SQLite's unlimited-limit form - an offset with no limit still + # needs a LIMIT clause for OFFSET to take effect. + sql += " LIMIT -1 OFFSET ?" + params.append(offset) + + cur.execute(sql, params) rows = cur.fetchall() events = [row_to_json(list(r.keys()), r) for r in rows] diff --git a/server/plugin.py b/server/plugin.py index 79de388da..181463cbd 100755 --- a/server/plugin.py +++ b/server/plugin.py @@ -1014,11 +1014,22 @@ def process_plugin_events(db, plugin, plugEventsArr): columnsStr = columnsStr[1:] valuesStr = valuesStr[1:] + # Destination CurrentScan columns that hold a MAC address. + # scanMac is normally already lowercase by this point - plugin_object_class.__init__ + # (below) normalizes objectPrimaryId via primary_id_is_mac(), and every current + # CurrentScan-mapped plugin declares "type": "device_mac"/"device_name_mac" on that + # column - so this is defense-in-depth for a plugin that omits that type annotation. + # scanParentMAC has no equivalent upstream normalization at all (primary_id_is_mac() + # only ever checks objectPrimaryId), so this is the only place it gets normalized. + _MAC_COLUMNS = ("scanMac", "scanParentMAC") + # Map the column names to plugin object event values and create a list of tuples 'sqlParams'. for plgEv in pluginEvents: tmpList = [] for col in mappedCols: + _tmpList_len_before = len(tmpList) + if col["column"] == "index": tmpList.append(plgEv.index) elif col["column"] == "plugin": @@ -1062,6 +1073,11 @@ def process_plugin_events(db, plugin, plugEventsArr): ): tmpList.append(col["mapped_to_column_data"]["value"]) + if dbTable == "CurrentScan" and col.get("mapped_to_column") in _MAC_COLUMNS: + for _i in range(_tmpList_len_before, len(tmpList)): + if tmpList[_i]: + tmpList[_i] = normalize_mac(tmpList[_i]) + # Append the mapped values to the list 'sqlParams' as a tuple. sqlParams.append(tuple(tmpList)) diff --git a/server/workflows/triggers.py b/server/workflows/triggers.py index fb365ad42..750246079 100755 --- a/server/workflows/triggers.py +++ b/server/workflows/triggers.py @@ -42,12 +42,12 @@ def __init__(self, triggerJson, event, db): query = f""" SELECT * FROM {db_table} - WHERE {refField} = '{event["objectGuid"]}' + WHERE {refField} = ? """ mylog("trace", [query]) - result = db.sql.execute(query).fetchall() + result = db.sql.execute(query, (event["objectGuid"],)).fetchall() if len(result) > 0: self.object = result[0] diff --git a/test/api_endpoints/test_devices_endpoints.py b/test/api_endpoints/test_devices_endpoints.py index ce7c3523e..26c1a4914 100644 --- a/test/api_endpoints/test_devices_endpoints.py +++ b/test/api_endpoints/test_devices_endpoints.py @@ -263,6 +263,53 @@ def test_devices_by_status_pagination(client, api_token): delete_dummy(client, api_token, mac) +def test_get_all_devices_pagination(client, api_token): + """limit/offset on GET /devices must page through the same set the + unpaginated response gives, ordered by devMac, with no gaps or + duplicates, and must reject invalid values.""" + macs = [f"aa:bb:cc:dd:ff:0{i}" for i in (1, 2, 3)] + for mac in macs: + create_dummy(client, api_token, mac) + + try: + full_resp = client.get("/devices", headers=auth_headers(api_token)) + assert full_resp.status_code == 200 + full_macs = [d["devMac"] for d in full_resp.json["devices"]] + assert set(macs).issubset(set(full_macs)) + + total = len(full_macs) + half = (total + 1) // 2 + page1 = client.get( + f"/devices?limit={half}&offset=0", headers=auth_headers(api_token) + ).json["devices"] + page2 = client.get( + f"/devices?limit={total - half}&offset={half}", + headers=auth_headers(api_token), + ).json["devices"] + paged_macs = [d["devMac"] for d in page1] + [d["devMac"] for d in page2] + assert paged_macs == full_macs + + # offset alone (no limit) must still take effect. + offset_only = client.get( + f"/devices?offset={half}", headers=auth_headers(api_token) + ).json["devices"] + assert [d["devMac"] for d in offset_only] == full_macs[half:] + + # Invalid limit/offset are rejected, not silently clamped. + resp_bad_limit = client.get( + "/devices?limit=0", headers=auth_headers(api_token) + ) + assert resp_bad_limit.status_code == 422 + + resp_bad_offset = client.get( + "/devices?offset=-1", headers=auth_headers(api_token) + ) + assert resp_bad_offset.status_code == 422 + finally: + for mac in macs: + delete_dummy(client, api_token, mac) + + def test_delete_test_devices(client, api_token): # Delete by MAC diff --git a/test/api_endpoints/test_events_endpoints.py b/test/api_endpoints/test_events_endpoints.py index 4b613ced8..41b8bea98 100644 --- a/test/api_endpoints/test_events_endpoints.py +++ b/test/api_endpoints/test_events_endpoints.py @@ -131,6 +131,86 @@ def test_delete_all_events(client, api_token, test_mac): assert len(resp.json.get("events", [])) == 0 +def test_get_events_pagination(client, api_token, test_mac): + """limit/offset on GET /events must page through the same set the + unpaginated response gives for one MAC, ordered by eveDateTime + descending, with no gaps or duplicates, and must reject invalid values.""" + # Distinct event_type per call - idx_events_unique is on + # (eveMac, eveIp, eveEventType, eveDateTime), and eveDateTime only has + # second precision, so 5 calls in the same second with the same + # event_type would collide and INSERT OR IGNORE would drop 4 of them. + for i in range(5): + create_event(client, api_token, test_mac, event=f"UnitTest Event {i}") + + full_resp = list_events(client, api_token, test_mac) + assert full_resp.status_code == 200 + full_events = full_resp.json.get("events", []) + assert len(full_events) >= 5 + + total = len(full_events) + half = (total + 1) // 2 + page1 = client.get( + f"/events?mac={test_mac}&limit={half}&offset=0", + headers=auth_headers(api_token), + ).json.get("events", []) + page2 = client.get( + f"/events?mac={test_mac}&limit={total - half}&offset={half}", + headers=auth_headers(api_token), + ).json.get("events", []) + assert page1 + page2 == full_events + + # offset alone (no limit) must still take effect. + offset_only = client.get( + f"/events?mac={test_mac}&offset={half}", + headers=auth_headers(api_token), + ).json.get("events", []) + assert offset_only == full_events[half:] + + # Invalid limit/offset are rejected, not silently clamped. + resp_bad_limit = client.get( + f"/events?mac={test_mac}&limit=0", headers=auth_headers(api_token) + ) + assert resp_bad_limit.status_code == 422 + + resp_bad_offset = client.get( + f"/events?mac={test_mac}&offset=-1", headers=auth_headers(api_token) + ) + assert resp_bad_offset.status_code == 422 + + +def test_get_events_pagination_stable_order_for_ties(client, api_token, test_mac): + """Events sharing the exact same eveDateTime (a real occurrence - it only + has second precision) must still page deterministically: ORDER BY + eveDateTime DESC alone leaves tied rows in an unspecified order, so + concatenated pages could omit or duplicate rows. rowid DESC as a secondary + key must make the order stable across the unpaginated and paged calls.""" + # create_event() only sets event_time when days_old is given, so post + # directly with an explicit, identical event_time for all 5 to force a tie. + shared_time = timeNowUTC(as_string=False).isoformat() + for i in range(5): + payload = {"ip": "0.0.0.0", "event_type": f"TieEventExplicit {i}", "event_time": shared_time} + resp = client.post(f"/events/create/{test_mac}", json=payload, headers=auth_headers(api_token)) + assert resp.status_code == 200 + + full_resp = list_events(client, api_token, test_mac) + full_events = full_resp.json.get("events", []) + tied = [e for e in full_events if e.get("eveEventType", "").startswith("TieEventExplicit")] + assert len(tied) == 5 + assert all(e["eveDateTime"] == tied[0]["eveDateTime"] for e in tied) + + total = len(full_events) + half = (total + 1) // 2 + page1 = client.get( + f"/events?mac={test_mac}&limit={half}&offset=0", + headers=auth_headers(api_token), + ).json.get("events", []) + page2 = client.get( + f"/events?mac={test_mac}&limit={total - half}&offset={half}", + headers=auth_headers(api_token), + ).json.get("events", []) + assert page1 + page2 == full_events + + def test_delete_events_dynamic_days(client, api_token, test_mac): # Determine initial count so test doesn't rely on preexisting events before = list_events(client, api_token, test_mac) diff --git a/test/api_endpoints/test_mcp_extended_endpoints.py b/test/api_endpoints/test_mcp_extended_endpoints.py index 49daeef5d..48975f8cf 100644 --- a/test/api_endpoints/test_mcp_extended_endpoints.py +++ b/test/api_endpoints/test_mcp_extended_endpoints.py @@ -177,7 +177,7 @@ def test_get_all_events(mock_get, client, api_token): response = client.get('/events?mac=00:11:22:33:44:55', headers=auth_headers(api_token)) assert response.status_code == 200 assert response.json["success"] is True - mock_get.assert_called_with("00:11:22:33:44:55") + mock_get.assert_called_with("00:11:22:33:44:55", None, None) @patch('models.event_instance.EventInstance.deleteEventsOlderThan') diff --git a/test/backend/test_workflows.py b/test/backend/test_workflows.py index 73cb21326..fba81deb4 100644 --- a/test/backend/test_workflows.py +++ b/test/backend/test_workflows.py @@ -22,7 +22,10 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server")) sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) -from db_test_helpers import make_db, make_device_dict, insert_device_from_dict +from db_test_helpers import ( + make_db, make_device_dict, insert_device_from_dict, + CREATE_PLUGINS_OBJECTS, CREATE_PLUGINS_HISTORY, +) # --------------------------------------------------------------------------- @@ -465,5 +468,56 @@ def test_condition_on_event_field_still_works_when_object_missing(self): self.assertTrue(condition.evaluate(trigger)) +class TestTriggerDeviceGuidLookup(unittest.TestCase): + """Trigger.__init__'s Devices/devGUID lookup (workflows/triggers.py) - + covers the parameterized query and the idx_dev_guid index added + alongside it.""" + + def setUp(self): + from types import SimpleNamespace + self.conn = make_db() + self.db = SimpleNamespace(sql=self.conn) + dev = make_device_dict("aa:bb:cc:dd:ee:01", devGUID="guid-a") + insert_device_from_dict(self.conn, dev) + + def test_lookup_finds_matching_device(self): + from workflows.triggers import Trigger + + event = _make_app_event(obj_guid="guid-a", obj_type="Devices", event_type="update") + trigger = Trigger({"object_type": "Devices", "event_type": "update"}, event, self.db) + + self.assertIsNotNone(trigger.object) + self.assertEqual(trigger.object["devMac"], "aa:bb:cc:dd:ee:01") + + def test_query_is_parameterized_not_string_interpolated(self): + """A devGUID containing a single quote must not raise + sqlite3.OperationalError - regression guard against reverting to + f-string interpolation of event['objectGuid'].""" + from workflows.triggers import Trigger + + event = _make_app_event(obj_guid="a'b", obj_type="Devices", event_type="update") + trigger = Trigger({"object_type": "Devices", "event_type": "update"}, event, self.db) + + self.assertIsNone(trigger.object) + + def test_devguid_lookup_uses_index(self): + from db.db_upgrade import ensure_Indexes + + # ensure_Indexes() also indexes Plugins_Objects.objectGuid and + # Plugins_History(plugin, dateTimeChanged), neither of which + # make_db()'s minimal fixture creates. + self.conn.execute(CREATE_PLUGINS_OBJECTS) + self.conn.execute(CREATE_PLUGINS_HISTORY) + ensure_Indexes(self.conn) + + plan = self.conn.execute( + "EXPLAIN QUERY PLAN SELECT * FROM Devices WHERE devGUID = ?", ("guid-a",) + ).fetchall() + plan_text = " ".join(str(tuple(row)) for row in plan) + + self.assertIn("idx_dev_guid", plan_text) + self.assertNotIn("SCAN Devices", plan_text) + + if __name__ == "__main__": unittest.main() diff --git a/test/db/test_devices_tiles.py b/test/db/test_devices_tiles.py new file mode 100644 index 000000000..6f4a3a252 --- /dev/null +++ b/test/db/test_devices_tiles.py @@ -0,0 +1,118 @@ +""" +Unit tests for get_sql_devices_tiles() (server/db/db_helper.py). + +Tests verify that: +- Tile counts match hand-computed expectations for a known device set. +- The query evaluates DevicesView exactly once, not once per tile - a + regression guard against reintroducing the 9-independent-scalar-subquery + shape that re-ran DevicesView's own per-row cost (the devFlapping + correlated EXISTS) 9 times per call. +""" + +import sys +import os + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +from db_test_helpers import ( # noqa: E402 + make_db, + make_device_dict, + insert_device_from_dict, +) + +from db.db_helper import get_sql_devices_tiles # noqa: E402 + + +def _seed_devices(conn): + """5 devices with distinct, known statuses.""" + devices = [ + make_device_dict("aa:bb:cc:dd:ee:01", devPresentLastScan=1, devIsArchived=0), + make_device_dict("aa:bb:cc:dd:ee:02", devPresentLastScan=1, devIsArchived=0, + devFavorite=1), + make_device_dict("aa:bb:cc:dd:ee:03", devPresentLastScan=0, devIsArchived=0), + make_device_dict("aa:bb:cc:dd:ee:04", devPresentLastScan=0, devIsArchived=0, + devIsNew=1), + make_device_dict("aa:bb:cc:dd:ee:05", devPresentLastScan=0, devIsArchived=1), + ] + for d in devices: + insert_device_from_dict(conn, d) + conn.commit() + + +class TestDevicesTilesCounts: + def test_tile_counts_match_expected(self): + conn = make_db() + try: + _seed_devices(conn) + conn.execute( + "INSERT INTO Settings (setKey, setValue) VALUES ('UI_MY_DEVICES', ?)", + ("['online','offline']",), + ) + conn.commit() + + row = conn.execute(get_sql_devices_tiles()).fetchone() + cols = [d[0] for d in conn.execute(get_sql_devices_tiles()).description] + tiles = dict(zip(cols, row)) + + # devices 1,2 present -> connected; 3,4 present=0,archived=0 -> offline; + # 5 archived -> excluded from active counts, counted only in archived/all_devices + assert tiles["connected"] == 2 + assert tiles["offline"] == 2 + assert tiles["archived"] == 1 + assert tiles["favorites"] == 1 + assert tiles["new"] == 1 + assert tiles["all"] == 4 # active (non-archived) devices + assert tiles["all_devices"] == 5 # every device, including archived + # UI_MY_DEVICES = online+offline -> connected(2) + offline(2) + assert tiles["my_devices"] == 4 + finally: + conn.close() + + def test_empty_devicesview_returns_zero_not_null(self): + """Regression guard: SUM(CASE...) over a zero-row DevicesView/Statuses + cross join returns NULL per column, not 0 - COALESCE(..., 0) must be + wrapped around every SUM-based tile, or a fresh install with no + devices yet would see null tile counts instead of zeros.""" + conn = make_db() + try: + row = conn.execute(get_sql_devices_tiles()).fetchone() + cols = [d[0] for d in conn.execute(get_sql_devices_tiles()).description] + tiles = dict(zip(cols, row)) + + # Iterate the query's own output columns rather than a separately + # hardcoded key list, so this stays correct if a tile is renamed + # or added/removed in get_sql_devices_tiles() itself. + for key, value in tiles.items(): + assert value == 0, f"{key} was {value!r}, expected 0" + finally: + conn.close() + + def test_devicesview_evaluated_once_not_per_tile(self): + """Regression guard: the query must not re-scan/re-evaluate DevicesView + once per tile column (the bug this rewrite fixed). SQLite's planner + flattens the view and reports the underlying 'Devices' table in the + plan rather than 'DevicesView' itself - count SCAN/SEARCH operations + on either name, not the literal string 'DevicesView'.""" + conn = make_db() + try: + _seed_devices(conn) + conn.execute( + "INSERT INTO Settings (setKey, setValue) VALUES ('UI_MY_DEVICES', ?)", + ("['online']",), + ) + conn.commit() + + plan = conn.execute( + "EXPLAIN QUERY PLAN " + get_sql_devices_tiles() + ).fetchall() + plan_lines = [str(tuple(row)) for row in plan] + device_scans = [ + line for line in plan_lines + if ("SCAN Devices" in line or "SEARCH Devices" in line) + ] + + assert len(device_scans) == 1, ( + f"expected exactly 1 scan/search of Devices(View), got " + f"{len(device_scans)}: {plan_lines}" + ) + finally: + conn.close() diff --git a/test/db_test_helpers.py b/test/db_test_helpers.py index edf10ffa7..646f8526d 100644 --- a/test/db_test_helpers.py +++ b/test/db_test_helpers.py @@ -664,7 +664,7 @@ def make_plugin_event_row(prefix: str, primary_id: str, secondary_id="sec", watched1="val1", watched2="", watched3="", watched4="", changed="2026-01-01 00:00:00", extra="", user_data="", foreign_key="", - status="not-processed"): + status="not-processed", help_val1=None): """Build a tuple mimicking a raw plugin output row (19 columns + index).""" return ( 0, # index (placeholder, not used for events) @@ -682,7 +682,7 @@ def make_plugin_event_row(prefix: str, primary_id: str, secondary_id="sec", user_data, foreign_key, None, # syncHubNodeName - None, # helpVal1 + help_val1, None, # helpVal2 None, # helpVal3 None, # helpVal4 diff --git a/test/scan/test_currentscan_mac_normalization.py b/test/scan/test_currentscan_mac_normalization.py new file mode 100644 index 000000000..4be824ab3 --- /dev/null +++ b/test/scan/test_currentscan_mac_normalization.py @@ -0,0 +1,106 @@ +""" +Tests for MAC normalization on the CurrentScan-promotion path +(server/plugin.py:process_plugin_events()). + +scanMac is normally already lowercase by the time this code runs - +plugin_object_class.__init__ normalizes objectPrimaryId via +primary_id_is_mac(), and every current CurrentScan-mapped plugin declares +"type": "device_mac"/"device_name_mac" on that column - so covering it here +too is defense-in-depth for a plugin that omits that type annotation. + +scanParentMAC has no such upstream normalization at all (primary_id_is_mac() +only ever checks objectPrimaryId) - this is the column these tests actually +exist to cover, since none of the ~22 real plugins mapping to it call +normalize_mac() in their own script.py. +""" + +import sys +import os + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +from db_test_helpers import ( # noqa: E402 + make_plugin_db, + make_plugin_dict, + make_plugin_event_row, + CREATE_CURRENT_SCAN, +) + +from plugin import process_plugin_events # noqa: E402 + +PREFIX = "TESTPLG" + + +def _plugin_with_parentmac_mapping(prefix: str) -> dict: + """A plugin dict mapping objectPrimaryId -> scanMac and helpVal1 -> scanParentMAC, + matching the real shape used by e.g. unifi_import/omada_sdn_imp/rest_import.""" + plugin = make_plugin_dict(prefix) + plugin["mapped_to_table"] = "CurrentScan" + plugin["database_column_definitions"] = [ + {"column": "objectPrimaryId", "mapped_to_column": "scanMac", "type": "device_mac"}, + {"column": "helpVal1", "mapped_to_column": "scanParentMAC"}, + ] + return plugin + + +def _current_scan_row(conn): + cur = conn.cursor() + cur.execute("SELECT scanMac, scanParentMAC FROM CurrentScan") + return cur.fetchone() + + +def _plugin_db(): + db, conn = make_plugin_db() + conn.execute(CREATE_CURRENT_SCAN) + conn.commit() + return db, conn + + +class TestScanMacNormalization: + def test_uppercase_scanmac_is_lowercased(self): + db, conn = _plugin_db() + try: + plugin = _plugin_with_parentmac_mapping(PREFIX) + row = make_plugin_event_row(PREFIX, "AA:BB:CC:DD:EE:01") + process_plugin_events(db, plugin, [row]) + + scan_mac, _ = _current_scan_row(conn) + assert scan_mac == "aa:bb:cc:dd:ee:01" + finally: + conn.close() + + +class TestScanParentMacNormalization: + """The real coverage gap: scanParentMAC has no upstream normalization, + unlike scanMac (see module docstring).""" + + def test_uppercase_scanparentmac_is_lowercased(self): + db, conn = _plugin_db() + try: + plugin = _plugin_with_parentmac_mapping(PREFIX) + row = make_plugin_event_row( + PREFIX, "aa:bb:cc:dd:ee:01", help_val1="AA:BB:CC:DD:EE:99" + ) + process_plugin_events(db, plugin, [row]) + + _, parent_mac = _current_scan_row(conn) + assert parent_mac == "aa:bb:cc:dd:ee:99" + finally: + conn.close() + + def test_empty_scanparentmac_left_empty_not_corrupted(self): + """normalize_mac(None)/normalize_mac('') must not be applied to a + falsy value - guards the truthy check in process_plugin_events()'s + normalization step against turning an unset parent MAC into garbage + (e.g. normalize_mac(None) would otherwise produce 'no:ne').""" + db, conn = _plugin_db() + try: + plugin = _plugin_with_parentmac_mapping(PREFIX) + row = make_plugin_event_row( + PREFIX, "aa:bb:cc:dd:ee:01", help_val1="" + ) + process_plugin_events(db, plugin, [row]) + + _, parent_mac = _current_scan_row(conn) + assert parent_mac == "" + finally: + conn.close()