Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude/skills/database-patterns/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ description: Read before designing a feature that writes to the Devices table, a

Before implementing any feature that reads or writes the `Devices` table, audit ALL write paths. The table is modified from many locations — missing one path is a correctness bug.

**Known production write paths (as of 2026-07-04):**
**Known production write paths:**

| File | Function | Fields written |
|---|---|---|
Expand Down
5 changes: 3 additions & 2 deletions .claude/skills/plugin-development/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ server/plugins/<code_name>/
- `<PREF>_CMD`: script path.
- `<PREF>_RUN_TIMEOUT`: timeout in seconds — **enforced by the core plugin runner as the whole script's kill-timeout** (`server/plugin.py` passes it straight to `subprocess(..., timeout=...)`). Not a safe per-HTTP-call timeout — don't reuse it for individual network calls in a loop, or one slow call can burn the whole budget and get the process killed before it writes its result file. Two correct alternatives: `config.json`'s `"timeoutMultiplier": true` on a `params[]` entry for a config-declared, known-length loop (see `arp_scan`); `plugin_helper.per_item_timeout()` for a runtime-variable-length loop (see the `_publisher_*` plugins).
- `<PREF>_WATCH`: columns to watch for changes.
- `<PREF>_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_DEV_DATA_CONTRACT.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns.
- `<PREF>_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_IMPORT_BEHAVIOR.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns.
- **`dataType` and `default_value` must agree.** `dataType: "array"`/`"object"` needs a real JSON literal for `default_value` (`'["default"]'`), not a bare string (`"default"`). `setting_value_to_python_type()` (`server/helper.py`) `json.loads()`s the default at runtime; a bare string fails silently — logged, and `[]` is returned instead of your default (e.g. `devParentRelType`, `UI_theme`, `UI_TOPOLOGY_ORDER`). If `elementOptions` already sets `multiple`/`orderable: "false"`, the setting is scalar — use `dataType: "string"` instead.

## Data Contract

Expand Down Expand Up @@ -70,7 +71,7 @@ Full column spec: `docs/PLUGINS_DEV_DATA_CONTRACT.md`. Note `helpVal1-4`/`watche

## Before Opening a PR

Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings instead of duplicating them, description length (renders in the Settings UI — keep it short), and the multi-instance settings pattern (nested array + popup-form, see `rest_import`, not a hardcoded "primary"/"secondary" pair). Most plugin PR review comments trace back to one of these, and `test/plugins/test_plugin_conventions.py` mechanically enforces the RUN-default, description-length, hardcoded-default-drift, and RUN_TIMEOUT-reuse-in-loop items — run it after touching a plugin.
Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings instead of duplicating them, description length (renders in the Settings UI — keep it short), and the multi-instance settings pattern (nested array + popup-form, see `rest_import`, not a hardcoded "primary"/"secondary" pair). Most plugin PR review comments trace back to one of these, and `test/plugins/test_plugin_conventions.py` mechanically enforces the RUN-default, description-length, hardcoded-default-drift, RUN_TIMEOUT-reuse-in-loop, and array/object dataType-default_value-mismatch items — run it after touching a plugin.

## Starting Point

Expand Down
2 changes: 1 addition & 1 deletion .claude/skills/plugin-readme/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Exception: call out a *specific* setting by name, in prose, only when its behavi

## Verify against the actual code first

Read `config.json` (`unique_prefix`, `plugin_type`, `data_source`, `settings`) and the plugin's script before writing anything - don't guess at mechanism from the plugin's name alone. Real bugs found this way during a past audit: `dig_scan/README.md` described the `nbtscan` utility (copy-paste from a sibling plugin); `adguard_import/README.md` was a byte-for-byte copy of `__template/README.md`, never actually written.
Read `config.json` (`unique_prefix`, `plugin_type`, `data_source`, `settings`) and the plugin's script before writing anything - don't guess at mechanism from the plugin's name alone. A README copied from a sibling plugin or left as the unedited `__template/README.md` describes the wrong plugin's behavior.

## Backfilling missing "Other info"

Expand Down
10 changes: 5 additions & 5 deletions .claude/skills/prd-writing/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ Triggered by: "write a PRD", "draft a design doc", "spec out this feature", "cre

## Core principle: a PRD is a claim-verification exercise, not a writing exercise

Every sentence that asserts something about how the code currently works must be checked against the actual code before it goes in — not written from memory, not inferred from a plugin's name or reputation, not assumed because it sounds plausible. Two real, caught-in-review examples from this exact process:
Every sentence that asserts something about how the code currently works must be checked against the actual code before it goes in — not written from memory, not inferred from a plugin's name or reputation, not assumed because it sounds plausible. Two failure patterns to watch for:

- A draft claimed "plugin X's rows are the cleanest case for a static presence-flag value" — reasonable-sounding, and wrong. Reading the actual script showed it already reports a live per-row state field and computes an equivalent boolean internally that was simply never wired up. The claim was never checked against the script, only against the plugin's category ("reservation-style").
- A draft claimed "no changes needed here" for two queries when adding a new presence column, on the reasoning that they were already "inert" for the new case. True for one sub-case (a device that starts absent), false for the transition sub-case (a device going from online to newly-suppressed) — because those two queries used a different existence check than the one already patched. It was missed for a full turn, until a direct question ("does this handle the transition where a device *was* online?") forced a re-trace of the actual call graph.
- Calling a plugin "the cleanest case" for some behavior based on its category alone. Read the actual script it may already report a live per-row signal and compute an equivalent value internally that was never wired up.
- Claiming "no changes needed" for a query based on one sub-case (e.g. a device that starts absent) without checking the transition sub-case (a device going from online to newly-suppressed) — the two sub-cases can use different existence checks, so a fix covering one can silently miss the other.

Both mistakes were plausible, well-written, and wrong. Neither would have survived actually reading the code first.
Both are plausible, well-written, and wrong. Reading the code first catches both.

## Process

Expand All @@ -28,7 +28,7 @@ Both mistakes were plausible, well-written, and wrong. Neither would have surviv
6. **Force every open question to an explicit decision**, even if the decision is "accept as-is for v1, revisit if feedback says otherwise." An open question left unresolved in a PRD gets silently decided by whoever implements it — usually differently than anyone actually intended.
7. **Write the test plan as part of the PRD, not after.** Concrete test cases — naming real functions/queries, not "add tests for X" — force you to notice design gaps you'd otherwise miss; the moment you try to write "assert Y happens" and realize the current design can't produce Y is often the first time the gap becomes visible. Check the repo for an existing test pattern for this shape of change before inventing a new one (e.g. a prior presence-logic bug fixed via `test/db_test_helpers.py` fixtures is the template for the next one, not a reason to build new test infrastructure).
8. **Ask explicitly whether validating this needs real end-to-end infrastructure** (a new or modified plugin, a UI click-through) or whether synthetic unit-level fixtures suffice — don't assume either way. Check whether the functions under test take a DB connection/dict/list as a parameter (testable in isolation, no real plugin needed) or require a real file on disk (harder to fake, may need one).
9. **Evaluate performance impact against the schema that actually exists, not the schema you'd expect, and against real deployment scale, not an imagined one.** For every new or modified query: does it reuse an existing index, or does it add an unindexed lookup, a new join, or a correlated subquery? Check for real — don't assume a column is indexed just because it looks identity-like (`CurrentScan.scanMac` looked like exactly the kind of column that should have an index; grepping for `CREATE INDEX` showed this codebase never gave it one, and neither did the first draft of the design that needed it — since fixed, `idx_currentscan_scanmac` now exists in `server/db/db_upgrade.py:ensure_CurrentScan()`, so check whether a later PRD's problem is already mitigated before assuming it's new). Then multiply the per-query cost by two things: how often it runs (a full scan inside a loop that fires once is nothing; the same scan inside a cycle that reruns every few minutes forever is a standing cost, permanently), and the actual scale this project runs at — **known real production users run 10,000+ devices** (confirmed directly by the project owner, not a guess or an inference from `CLAUDE.md`'s "homelabs, MSPs, and NOCs" framing). At that scale, a `CurrentScan` populated at 2-5 rows per device (one per contributing plugin, the normal case) is routinely 20,000-50,000+ rows in a single cycle — treat that as the number to reason about, not a hypothetical upper bound reserved for some future large deployment. Concrete example from this process: implementing a new multi-source precedence rule as a correlated `EXISTS` subquery re-evaluated per candidate row reads as perfectly reasonable, passes every test at small scale, and is an accidental self-join with no index behind it at scale — the fix (add the missing index, express the aggregation as one `GROUP BY` pass instead of a per-row correlated check) had to be written into the PRD explicitly, or it would have shipped as a footgun that real 10k-device users would have hit, not a theoretical one.
9. **Check performance against the real schema and real scale, not assumptions.** For every new or changed query: does it use an existing index, or add an unindexed lookup, a new join, or a correlated subquery? Grep `CREATE INDEX` — don't assume a column is indexed just because it looks like a key (check `server/db/db_upgrade.py`/`server/db/schema/app.sql`). Then weigh cost by how often the query runs (once is nothing; every few minutes forever is a standing cost) and by real scale — **production users run 10,000+ devices**, not a homelab handful. A `CurrentScan` with 2-5 rows per device (one per contributing plugin) is routinely 20,000-50,000+ rows in one cycle; reason about that number, not a smaller hopeful one. A correlated `EXISTS`/subquery re-evaluated per outer row is fine *if* the correlated column is indexed — e.g. `current_scan_presence_condition()` (`server/scan/presence.py`) is exactly this shape against `CurrentScan.scanMac`, covered by `idx_currentscan_scanmac`. The real risk is an unindexed correlated lookup: an accidental self-join scanning the full inner table per outer row, which looks fine and passes tests at small scale but isn't at production scale. Check with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming either way; if it comes back unindexed, a `GROUP BY` aggregate is the usual fix.
10. **Do a dedicated final-check pass, out loud, before calling it done.** Re-read the whole document end to end and specifically check:
- Did a correction made mid-document actually propagate everywhere it needed to (the Design subsection *and* Affected Files *and* Tests *and* any execution-plan summary)? A correction landing in one place and not its siblings is worse than never catching it, because now the document silently contradicts itself.
- Does every "this is the cleanest/simplest real case" claim still hold up if you actually re-read that specific piece of code right now, or was it asserted by pattern-matching a name/category? Re-verify, don't re-assert.
Expand Down
Loading
Loading