Addresses CodeRabbit review on PR #1765 (pullrequestreview-5069337680).
pihole_monitor.py:
- last_raw is now tracked per source ({"primary": N, "secondary": M}
per device) instead of one combined value. Combining raw totals
across sources before diffing let a counter reset on one instance
silently net out against real traffic on the other - e.g. primary
+2000 (a real spike) and secondary resetting 1000->5 (-995) would
combine into a raw delta of only 1005, hiding most of the primary's
actual spike behind the secondary's unrelated restart.
- New aggregate_source_deltas(): diffs each source independently via
compute_delta(), then sums only the valid deltas. A source with no
valid delta this run (bootstrapping or just reset) contributes
nothing and doesn't block the others; each source keeps its own
reference point going forward.
- State loaded from before this change (last_raw as a plain number,
not per-source) is now tolerated instead of crashing - treated as no
prior reference point, so every source just bootstraps fresh on the
next run.
README.md:
- Fixed a self-contradicting line: a less frequent schedule means
larger per-run deltas, so PIHOLEMON_MIN_BLOCKED may need *raising*,
not lowering as it previously said.
- Corrected PIHOLEMON_HISTORY_DAYS guidance: it's a retention window,
not a detection delay. A new device becomes evaluable on its 3rd
successful run (1st anchors the counter, 2nd records the first
delta, 3rd has a baseline to compare against), not after the full
retention window.
Tests: 54 (up from 48). New coverage: aggregate_source_deltas() unit
tests including the exact dual-source reset-masking scenario, a
main()-level integration test for the same, and a regression test for
tolerating pre-per-source state. Both the reset-masking fix and the
legacy-state guard verified via mutation testing (reverted each,
confirmed the relevant tests fail, restored). 99% line+branch coverage
maintained.
References PR #1765.
Docs:
- Added PIHOLEMON to docs/PLUGINS.md and a new "Approach 4" section in
docs/PIHOLE_GUIDE.md, leading with anomaly detection (the actual
differentiator vs PIHOLEAPI) and explaining when to pick each plugin.
- README/PLUGINS.md/config.json's UI-facing description all reordered
and shortened to lead with anomaly detection instead of device
import, and to drop implementation detail that belongs in the
README, not the Settings page.
- Trimmed the "Why not extend PIHOLEAPI" README section per feedback -
useful context for a maintainer, not for an end user configuring
the plugin.
config.json / pihole_monitor.py:
- RUN defaults to "disabled", matching every other non-core plugin.
- VERIFY_SSL split into PRIMARY_VERIFY_SSL / SECONDARY_VERIFY_SSL -
each instance can be http/https independently. Settings reordered so
each *_VERIFY_SSL sits right under its matching *_PASSWORD.
- GRAPHQL_TOKEN removed; graphql_token now reads the core API_TOKEN
setting instead of a plugin-specific duplicate.
- GRAPHQL_URL replaced with a GET_OWNER boolean - the endpoint is now
derived from this app's own GRAPHQL_PORT (single source of truth)
instead of a URL the user had to keep in sync by hand.
- HISTORY_LENGTH (run count) replaced with HISTORY_DAYS (a real time
window): state now stores [timestamp, delta] samples and
trim_history() drops anything older than the window, so the
baseline means the same thing regardless of schedule - a faster
schedule adds more data points instead of shrinking the window.
- STATE_FILE moved from the log folder to dbFolderPath, so the rolling
anomaly baseline survives NetAlertX upgrades instead of being wiped
with the logs.
- netalertx_device_owner() (1 GraphQL call per device) replaced by
netalertx_device_owners() (1 call per run, batched) - avoids N
blocking round-trips on a large network.
- Fixed a zero-baseline bug: `bool(... and baseline and ...)` silently
exempted a device with an all-zero blocked-query history (0.0 is
falsy in Python) from ever being flagged, even on its first real
spike. Now checks `baseline is not None`.
- Fixed the placeholder-MAC filter: only excluded the literal "ip-::",
not Pi-hole's general "ip-<address>" placeholder pattern. Caught
downstream by is_mac() either way, but now the actual placeholder
check does what it looks like it does.
- Fixed a cumulative-counter bug: Pi-hole's /api/stats/top_clients
returns a count that's cumulative since FTL last started, not a
per-interval or daily-resetting one (confirmed against FTL's own
source and long-standing user reports that it doesn't reset at
midnight). Comparing that raw total directly against a rolling
average made any device's ordinary growing traffic look like an
escalating anomaly. compute_delta() now diffs each run's raw count
against the previous run's (state gained a per-key last_raw
reference point alongside the delta history) - None (not 0) on the
first-ever run for a device or right after a counter reset, so
those runs re-anchor the reference point instead of fabricating or
swallowing a delta.
- RUN_SCHD default changed from every 6 hours to every 5 minutes now
that the baseline window is real days, not run count, so a frequent
schedule only adds data points instead of narrowing the window; also
matches the default most other device-scanner plugins use.
- RUN_SCHD gained the same live cron-validity checkmark ARPSCAN and
other scanner plugins use (a ✓/✗ icon next to the field, validated
client-side against a regex) - reuses the existing generic
validateRegex() widget, nothing plugin-specific to build.
Tests: 48 tests (up from 37), 99% line+branch coverage. Every fix
above verified via mutation testing (deliberately broken, confirmed
the relevant test fails, then restored).
CodeRabbit follow-up on PR #1765
(https://github.com/netalertx/NetAlertX/pull/1765#discussion_r3888374014):
test_main_history_length_never_produces_empty_or_growing_unbounded only
asserted len(history) >= 1, which a mis-clamped history_length (e.g.
keeping 4 items instead of 1) would still pass unnoticed.
Replaced with test_main_history_length_clamps_and_trims_exactly,
seeding distinct ordered values and asserting the exact retained
history against each PIHOLEMON_HISTORY_LENGTH boundary. Verified it
actually catches a broken clamp: temporarily reverted the
max(1, ...) fix in pihole_monitor.py, confirmed this test fails
([] == [40]) while the rest of the suite still passes, then restored
the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CHJAArRiet4GmXUsxnNLdW
Addresses 5 of the 6 actionable comments from CodeRabbit's review of
PR #1765 (netalertx/NetAlertX#1765), plus adds test coverage:
- fetch_top_blocked_clients() returns None on failure instead of {},
so a failed request can no longer be mistaken for "genuinely zero
blocked queries this run" and silently write a false 0 into a
device's rolling history baseline. main() now tracks a
stats_complete flag and skips anomaly evaluation + history
persistence entirely for a run with incomplete blocked-query data.
- fetch_top_blocked_clients() is now called with count=max_clients
(the existing PIHOLEMON_API_MAXCLIENTS setting) instead of a
hardcoded default of 50, so clients beyond the top 50 are no longer
silently dropped from anomaly detection.
- New build_ip_to_mac() derives the IP->MAC identity map from every
gathered device entry instead of from merge_device_entries()'s
by-MAC-deduplicated output, which only kept one IP per device and
silently lost a multi-IP device's other IPs (misattributing their
blocked-query traffic to a bare IP instead of the real MAC).
- PIHOLEMON_HISTORY_LENGTH is clamped to at least 1, so a negative
setting can no longer reach the history[-history_length:] slice
with a nonsensical negative-of-negative length.
- PIHOLEMON_VERIFY_SSL now defaults to true (was false, matching the
official PIHOLEAPI plugin's convention). README documents the
http:// vs https:// credentials trade-off explicitly rather than
forcing https:// - most home Pi-hole setups, including the one this
plugin targets, run over plain HTTP on a trusted LAN.
- Added test/plugins/test_pihole_monitor.py (37 tests, 99% line and
branch coverage of pihole_monitor.py per pytest-cov - only the
`if __name__ == '__main__':` entry-point guard is unreached):
auth and deauth success/failure paths, the None-sentinel-on-failure
contract, fetch_devices()'s own failure path, build_ip_to_mac()'s
multi-IP fix, gather_device_entries()'s skip branches and fake-MAC
fallback, netalertx_device_owner()'s success/failure/no-URL paths,
and main()-level coverage for source aggregation, the
stats_complete gate, the history_length boundary clamp, the
CONSIDER_ONLINE fallback, an unconfigured-sources run, and the
offline-device / invalid-MAC / unknown-IP / owner-lookup branches
together in one run.
Not addressed: CodeRabbit's suggestion to hard-reject http:// URLs in
auth(). Diverges deliberately - it would break the plugin's majority
use case (Pi-hole admin API on a trusted home LAN without TLS), which
this repo's own PIHOLEAPI plugin also targets over plain HTTP.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CHJAArRiet4GmXUsxnNLdW
A custom header carrying a non-ASCII character raised UnicodeEncodeError from
inside http.client. That is a ValueError, not a RequestException, so it escaped
both handlers in send() and took the whole publisher down - every notification
lost because of one typo in one header.
build_custom_headers now rejects newlines and non-ASCII the same way it already
rejects malformed and colliding entries: warn, skip that entry, keep the rest.
UnicodeEncodeError is still caught at the request, as a backstop for the
plugin's own headers, since REPORT_DASHBOARD_URL feeds one of them.
Verified against real requests: before, send() raised UnicodeEncodeError; after,
the bad header is dropped and the notification is still posted.
The custom header added in #1695 was a single name/value pair, which is
enough for a proxy that authenticates with one token but not for Pangolin,
which expects both P-Access-Token-Id and P-Access-Token.
NTFY_CUSTOMHEADER_NAME and NTFY_CUSTOMHEADER_VALUE are replaced by a single
list setting, NTFY_CUSTOM_HEADERS, holding one "Name: Value" entry per
header. The list widget is the same one the other list settings use.
Only the first colon separates the name from the value, so values may
contain colons. An entry is skipped and logged when it is malformed, when
the name repeats, or when it collides with a header the plugin already set,
so a custom header still cannot clobber the ntfy credentials.
Values are never written to the log, since they are usually secrets. That
also applies to the invalid-header error, which now names the headers that
were applied without quoting any of them.
- Replace max() with explicit if/else for readability
- Use db_test_helpers (make_db, make_device_dict, insert_device_from_dict,
DummyDB) instead of local mock DB objects in test_nic_presence.py
- Lowercase all MAC addresses in tests
Co-authored-by: jokob-sk <96159884+jokob-sk@users.noreply.github.com>
When a parent device has NIC children, update_devPresentLastScan_based_on_nics
previously replaced the parent's devPresentLastScan unconditionally with the
NIC-derived value. This discarded any genuine direct detection of the parent:
if the parent was found by ARP/save_own_device (present=1) but its NIC child
was absent (present=0), the NIC step forced the parent back to 0. The next
scan re-detected the parent → Connected event → NIC forced it down again,
producing an endless one-directional Connected event stream.
Fix: use max(original, nic_derived) so NIC children can only raise a parent's
presence (bring an undetected parent online), never lower it when the parent
itself was directly detected this cycle.
Adds test/scan/test_nic_presence.py covering the exact regression scenario
and surrounding cases (raise, no-NIC unchanged, req_all modes).
Fixes#1736
Co-authored-by: jokob-sk <96159884+jokob-sk@users.noreply.github.com>
- Introduced a new plugin for monitoring website health, including functionality to check URLs and log results.
- Created README and configuration files for the workflows plugin, detailing its purpose and settings.
- Updated import paths in various test files to reflect the new directory structure.
- Ensured compatibility of test cases with the updated plugin architecture.
- Added db_history.py to manage DevicesHistory table and triggers for INSERT and UPDATE operations.
- Created device_history_instance.py for querying and grouping DevicesHistory records.
- Developed change_history.php for displaying device change history with filtering and pagination.
- Introduced skel_device_details_tab_history.php for skeleton loading state in device details tab.
- Added unit tests in test_device_history.py to validate trigger functionality and history management.
- Implemented filter population and pagination in the change history UI.
- Changed the test suite name for clarity.
- Updated default behavior for running all tests in the workflow.
- Improved logging in the Docker test script for better debugging.
- Modified assertions in Nginx proxy security tests to ensure access is not blocked.
- Added exception handling in UI tests to skip tests when elements are not found.
jokob-sk