diff --git a/.claude/skills/plugin-development/SKILL.md b/.claude/skills/plugin-development/SKILL.md index 834e69d5a..60101671c 100644 --- a/.claude/skills/plugin-development/SKILL.md +++ b/.claude/skills/plugin-development/SKILL.md @@ -61,6 +61,12 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext | `on_new_device` | When new device detected | | `on_notification` | When notification triggered | +## `all_plugins` Lifecycle - Cache Hot-Path Derivations by Identity + +`all_plugins` (every plugin's parsed `config.json`) is loaded once, in `initialise.py`'s `get_plugins_configs()` call, and reused as the same list object everywhere after that until an actual config reload replaces it. `update_api()` (`server/api.py`), which receives `all_plugins` as a parameter, runs far more often than that - on every scan cycle and every plugin completion (`server/__main__.py`, `server/plugin.py`), not just on reload. + +Any function deriving something from `all_plugins` that gets called from a hot path like `update_api()` should cache its result, not recompute on every call - the input is almost always the same object even though the call site fires constantly. `server/db/plugin_field_views.py`'s `get_all_device_field_views()` is the pattern: retain a reference to the last-seen `all_plugins` list and compare it to the current one with `is`, recomputing only when a genuinely new object (an actual reload) appears. Don't key the cache by `id(all_plugins)` alone - once the old list is garbage collected, Python can reuse that id for an unrelated object, which would wrongly serve a stale cached result for what is actually a new reload. + ## Plugin Formats | Format | Purpose | Phase | diff --git a/.claude/skills/prd-writing/SKILL.md b/.claude/skills/prd-writing/SKILL.md index 85facbd2f..636448622 100644 --- a/.claude/skills/prd-writing/SKILL.md +++ b/.claude/skills/prd-writing/SKILL.md @@ -53,11 +53,22 @@ Both are plausible, well-written, and wrong. Reading the code first catches both - **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. +- **Implementation checklist** — added to the PRD file itself when implementation actually starts (not at draft time), three boxes matching the "Implementing an approved PRD" section below: plan written before any code; test written and confirmed red against the pre-fix code before the fix was applied; no PRD filename/path/word left in committed code. Check each box only as it's actually done, not preemptively - the point is a forcing function visible in the file you're editing, not a formality. ## 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. +## Implementing an approved PRD + +Add the Implementation checklist (see Structure above) to the PRD file as the first edit, before touching any other code - an unchecked, visible checklist in the file you're actively working in is the forcing function; a rule read once at skill-load time is not. + +Before writing a single line of implementation code, write an explicit plan (via `EnterPlanMode` or an equivalent written plan) covering which files change, in what order, and which test(s) get written first. Understanding the PRD's design is not the same as planning its implementation - in practice these have been conflated, going straight from "I've read and understood this PRD" into code edits without ever writing either the plan or the test required below. + +Then follow step 7 above literally, not just in spirit, at implementation time too: write the test, run it against the pre-fix code, confirm it fails for the right reason (a real behavioral mismatch, or a legitimate missing-interface error for a brand-new contract - not a setup bug), and only then write the fix. Writing the test after the implementation and asserting it "would have failed" is not equivalent - it's an unverified claim. If you catch yourself having implemented first, the correct recovery is to actually revert the implementation, confirm the test fails red against the reverted code, and only then reapply the fix - not to just add tests afterward and call it done. + +Never reference a PRD's filename, path, or the word "PRD" in committed code, comments, docstrings, or commit messages - `PRDs/` is gitignored and local-only, so a reader of the committed code later has no way to find what it's pointing at, and the reference rots the moment the PRD file is deleted or moved. Explain the *why* self-containedly in the code itself (a design PRD's rationale restated in the comment, not a pointer to go read the PRD) instead. + ## 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 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 d1a5c1333..a7eeb0eb4 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -59,6 +59,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. 7. **`CurrentScan.scanMac`/`Events.eveMac` are permanently MAC-keyed, independent of the devGUID-as-PK migration.** See `database-patterns`' "Device Identity" section - `devMac` is today's actual schema PK, `devGUID` is the intended long-term identity, but a plugin can only ever report a MAC from network discovery, never an app-internal `devGUID`. Don't design around these tables ever becoming devGUID-keyed. +9. **`insert_events()`'s `IP Changed` query evaluates every distinct present address for a MAC individually, not one `MIN()`-reduced candidate.** A device can have more than one simultaneously-valid, already-known address in the same family (e.g. two live IPv6 addresses); testing only the `MIN()`'d one against `devPrimaryIPv4`/`devPrimaryIPv6`/`devLastIP` can pick the one address that happens not to be sitting in the single-valued slot, firing a false event even though nothing actually changed. The fix makes "known" additive: an address counts as known if it matches the three-slot check *or* if it's been reported (and isn't `missing-in-last-scan`) by an eligible plugin in `Plugins_Objects`, via `_known_plugin_ip_addresses_sql()`'s use of `plugin_field_views.get_plugin_columns_for_field(all_plugins, "ip")` - the same plugin-to-column mapping the Field View tab uses. `all_plugins` is optional on both `insert_events()` and `process_scan()` (default `None`); when it's not eligible for any plugin (or a caller hasn't passed it), the additive half is skipped entirely and the query reduces to exactly the three-slot check. Addresses that fail `known()` are aggregated into one `Events` row per MAC per cycle (today's shape), listing every new address in `eveAdditionalInfo` rather than firing one row per address. ## When to read this vs. other docs/skills diff --git a/.claude/skills/skill-hygiene/SKILL.md b/.claude/skills/skill-hygiene/SKILL.md index 1ec1e921f..0fe8617c1 100644 --- a/.claude/skills/skill-hygiene/SKILL.md +++ b/.claude/skills/skill-hygiene/SKILL.md @@ -57,4 +57,4 @@ This does not apply to PRDs (`.gemini/internal-docs/PRDs/`) — those keep their ## 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. +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` checks both: it flags a diff that touched some but not all of a group's paired files, and - for the groups the skills index documents as covering identical content - it also diffs the bodies directly and flags when they've actually drifted apart. The CI step calling it is non-blocking, so a manual diff after editing is still worth doing yourself. diff --git a/.claude/skills/ux-design-patterns/SKILL.md b/.claude/skills/ux-design-patterns/SKILL.md index 355691b62..1f8fa55a4 100644 --- a/.claude/skills/ux-design-patterns/SKILL.md +++ b/.claude/skills/ux-design-patterns/SKILL.md @@ -11,6 +11,30 @@ Don't introduce new UX behavior or visual patterns unless a PRD explicitly calls Real, recent example: a presence-page Prev/Next pager was first built with custom `