points the plugin-review skill at PIHOLEAPI's lastQuery check

The plugin-review skill, in its section on per-row scanPresence, named
"pihole_api_scan's lastSeen-vs-threshold check" as a real implementation
to follow. Pi-hole FTL refreshes ips[].lastSeen for every client it holds
in memory, so that field is exactly the signal that kept devices online
for about 24 hours. A reviewer following the skill would have been sent
to the pattern that caused the bug.

Changes:
- .claude/, .gemini/ and .github/skills/plugin-review/SKILL.md, same edit
  in all three copies:
  - the example list says "last-activity timestamp" instead of
    "last-seen timestamp",
  - the reference points to pihole_api_scan's lastQuery-vs-threshold
    check, with the file path,
  - one added sentence: a timestamp the upstream keeps refreshing for
    every client it merely remembers is not a recency signal.

Impact:
Documentation for reviewers only. The three copies stay identical
(scripts/check_skill_pairs.py reports nothing for plugin-review).
This commit is contained in:
sebingel committed 2026-11-01 09:30:41 +00:00
1 parent 98f5b81cc8
commit b0575f33b8
3 files changed
+3 -3

No files matched your search

+1 -1
View File
@@ -61,4 +61,4 @@ A worked example: a WiFi-scanning plugin PR added `iw` plus its `setcap` grant t
`scanPresence` defaults to `1` (`server/db/schema/app.sql`) when a plugin's `config.json` never maps it - every row that plugin reports asserts "currently online." That's correct for a plugin that only reports a device when it actually observed it *this cycle* (arp_scan, icmp_scan). It's wrong for a plugin whose data source is a **roster of known/historical entries** the upstream service remembers regardless of current connectivity (a client list built from DNS query history, a full device inventory export, anything that persists once discovered rather than expiring when the device goes quiet). Such a plugin re-reports the same entries every run whether or not the device is actually reachable, so the default `1` silently asserts permanent presence - read the actual data source (the API/file the script pulls from), not just the plugin's name, to tell which shape it is. `docs/PLUGINS_IMPORT_BEHAVIOR.md` already documents the fix (`scanPresence = 0` for "a reservation, a lease record, or a static IPAM entry"); this check exists because that guidance wasn't being verified against actual plugin behavior at review time.
If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-seen timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastSeen`-vs-threshold check and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes.
If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-activity timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastQuery`-vs-threshold check (`server/plugins/pihole_api_scan/pihole_api_scan.py`) and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A timestamp the upstream keeps refreshing for every client it merely remembers is not a recency signal: Pi-hole FTL refreshes `ips[].lastSeen` for every client it holds in memory, so `pihole_api_scan` reads the device's `lastQuery` (time of its last DNS query). A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes.
+1 -1
View File
@@ -61,4 +61,4 @@ A worked example: a WiFi-scanning plugin PR added `iw` plus its `setcap` grant t
`scanPresence` defaults to `1` (`server/db/schema/app.sql`) when a plugin's `config.json` never maps it - every row that plugin reports asserts "currently online." That's correct for a plugin that only reports a device when it actually observed it *this cycle* (arp_scan, icmp_scan). It's wrong for a plugin whose data source is a **roster of known/historical entries** the upstream service remembers regardless of current connectivity (a client list built from DNS query history, a full device inventory export, anything that persists once discovered rather than expiring when the device goes quiet). Such a plugin re-reports the same entries every run whether or not the device is actually reachable, so the default `1` silently asserts permanent presence - read the actual data source (the API/file the script pulls from), not just the plugin's name, to tell which shape it is. `docs/PLUGINS_IMPORT_BEHAVIOR.md` already documents the fix (`scanPresence = 0` for "a reservation, a lease record, or a static IPAM entry"); this check exists because that guidance wasn't being verified against actual plugin behavior at review time.
If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-seen timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastSeen`-vs-threshold check and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes.
If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-activity timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastQuery`-vs-threshold check (`server/plugins/pihole_api_scan/pihole_api_scan.py`) and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A timestamp the upstream keeps refreshing for every client it merely remembers is not a recency signal: Pi-hole FTL refreshes `ips[].lastSeen` for every client it holds in memory, so `pihole_api_scan` reads the device's `lastQuery` (time of its last DNS query). A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes.
+1 -1
View File
@@ -61,4 +61,4 @@ A worked example: a WiFi-scanning plugin PR added `iw` plus its `setcap` grant t
`scanPresence` defaults to `1` (`server/db/schema/app.sql`) when a plugin's `config.json` never maps it - every row that plugin reports asserts "currently online." That's correct for a plugin that only reports a device when it actually observed it *this cycle* (arp_scan, icmp_scan). It's wrong for a plugin whose data source is a **roster of known/historical entries** the upstream service remembers regardless of current connectivity (a client list built from DNS query history, a full device inventory export, anything that persists once discovered rather than expiring when the device goes quiet). Such a plugin re-reports the same entries every run whether or not the device is actually reachable, so the default `1` silently asserts permanent presence - read the actual data source (the API/file the script pulls from), not just the plugin's name, to tell which shape it is. `docs/PLUGINS_IMPORT_BEHAVIOR.md` already documents the fix (`scanPresence = 0` for "a reservation, a lease record, or a static IPAM entry"); this check exists because that guidance wasn't being verified against actual plugin behavior at review time.
If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-seen timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastSeen`-vs-threshold check and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes.
If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-activity timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastQuery`-vs-threshold check (`server/plugins/pihole_api_scan/pihole_api_scan.py`) and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A timestamp the upstream keeps refreshing for every client it merely remembers is not a recency signal: Pi-hole FTL refreshes `ips[].lastSeen` for every client it holds in memory, so `pihole_api_scan` reads the device's `lastQuery` (time of its last DNS query). A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes.