Reconcile optional endpoint availability from real requests - #81
Reconcile optional endpoint availability from real requests#81Snuffy2 wants to merge 15 commits into
Conversation
…-reconciliation # Conflicts: # aiopnsense/speedtest.py # tests/test_speedtest.py
📝 WalkthroughWalkthroughThe client now provides typed, authoritative results for optional endpoints, with method-aware caching, concurrency control, transport classification, DHCP state aggregation, and updated SMART, NUT, vnStat, Unbound, Speedtest, ARP, and lease APIs. Extensive tests cover schemas, state transitions, caching, transport behavior, and concurrency. ChangesOptional endpoint result model
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Documentation build overview
14 files changed ·
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dd2b25ba4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._store_confirmed_negative_endpoint_observation( | ||
| cache_key, f"confirmed_{method}_404" | ||
| ) |
There was a problem hiding this comment.
When get_smart_info() posts a device-specific payload and OPNsense returns 404 for that particular device (for example a stale/invalid disk name), this path-only negative observation is stored for /api/smart/service/info. Subsequent calls for other valid devices are then short-circuited as missing for the negative TTL even though the SMART endpoint itself is present, so payload-specific POST 404s should not poison the endpoint availability cache without a payload-independent confirmation.
Useful? React with 👍 / 👎.
| if response.status == 403: | ||
| _LOGGER.error( | ||
| "Permission Error in is_%s_endpoint_available. Path: %s. Ensure the OPNsense user connected to HA has appropriate access. Recommend full admin access", | ||
| normalized_method, | ||
| url, | ||
| ) |
There was a problem hiding this comment.
If this endpoint already has a fresh positive cache entry, a forced probe that now gets HTTP 403 reaches this branch but leaves the old True observation and timestamp intact. The next non-forced availability check will return the stale cached success until the positive TTL expires, so after an API key loses privileges the library can keep treating the endpoint as usable and issue failing follow-up requests; clear or replace the cache entry for permission failures before returning or raising.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e11c56e8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| if self._throw_errors: | ||
| raise _opnsense_http_error(response.status, response.reason) | ||
| return False |
There was a problem hiding this comment.
When a forced refresh hits a transient non-404 status such as 500 or 429 after a previous available cache entry, this branch returns False without removing or replacing that old entry. The next non-forced availability check will still take the fresh positive cache entry for up to the positive TTL, so callers continue treating an endpoint as usable even though the refresh just observed it failing; clear the cached observation before returning for these non-cacheable failures.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
aiopnsense/dhcp.py (1)
527-589: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftISC DHCPv4/DHCPv6 lease retrieval is fully duplicated (~60 lines twice).
Both blocks now duplicate the new
CategoryResult-based state-recording flow in addition to the pre-existing parsing duplication. A shared helper (parameterized similarly to_get_kea_dhcp_leases, taking the endpoint constants, theif/macfield names, andrequire_hardware_addresssemantics) would collapse this into one implementation and prevent v4/v6 from drifting, as already happened with the Kea variant.Also applies to: 601-662
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiopnsense/dhcp.py` around lines 527 - 589, Extract the duplicated ISC lease retrieval and parsing logic from the v4 and v6 paths into one shared helper, parameterized with the appropriate endpoint constants, interface and MAC field names, and require_hardware_address behavior, following the existing _get_kea_dhcp_leases pattern. Update both callers to use the helper while preserving CategoryResult state recording, malformed-response handling, expiration filtering, and returned lease fields.aiopnsense/unbound.py (1)
166-189: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLegacy malformed path returns a truthy
{"legacy": {}}, contradicting the "empty mapping" docstring contract.When
_get_unbound_blocklist_legacy()fails,get_unbound_blocklist_result()returnsCategoryResult({"legacy": {}}, "malformed", False), soget_unbound_blocklist()yields{"legacy": {}}— a non-empty, truthy dict. This contradicts the docstring's claim that malformed/unavailable results normalize to{}, and diverges from the extended path's genuinely empty{}on malformed. Callers relying on truthiness (a pattern already used elsewhere in this file) would treat this as present data.🐛 Proposed fix
legacy = await self._get_unbound_blocklist_legacy() if not legacy: - return CategoryResult({"legacy": {}}, "malformed", False) + return CategoryResult({}, "malformed", False) return CategoryResult({"legacy": legacy}, "available", True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiopnsense/unbound.py` around lines 166 - 189, Update the malformed legacy branch in get_unbound_blocklist_result so a falsy _get_unbound_blocklist_legacy() result produces an empty data mapping while retaining the "malformed" status and false availability metadata. Preserve the {"legacy": legacy} mapping for valid legacy data and keep get_unbound_blocklist() returning a truthy mapping only when blocklist data exists.tests/test_speedtest.py (1)
104-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConditional branches assert identical values — no real coverage of
showstat_availabledivergence.Both the
if showstat_available:andelse:branches (Lines 205-212) assert the exact sameresult["last"]values. Sinceshow_statresolves to{}in all three parametrized cases (even the "showstat-available" case supplies an empty payload), the test never actually exercises a scenario whereshowstat's data affects theaveragesection, making the conditional dead weight.♻️ Suggested improvement
- if showstat_available: - assert result["last"]["download"]["value"] == 1.0 - assert result["last"]["upload"]["value"] == 2.0 - assert result["last"]["latency"]["value"] == 3.0 - else: - assert result["last"]["download"]["value"] == 1.0 - assert result["last"]["upload"]["value"] == 2.0 - assert result["last"]["latency"]["value"] == 3.0 + # "last" values always come from showlog regardless of showstat state. + assert result["last"]["download"]["value"] == 1.0 + assert result["last"]["upload"]["value"] == 2.0 + assert result["last"]["latency"]["value"] == 3.0 + if showstat_available: + # supply a non-empty showstat payload in the parametrization to + # actually assert real "average" values here. + pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_speedtest.py` around lines 104 - 213, Update test_get_speedtest_probes_showstat_before_fetching_optional_payload so the showstat-available parameter supplies a non-empty payload that changes the expected result, then assert that distinct showstat_available branches validate the showstat-derived values versus the fallback values. Remove the redundant identical assertions while preserving the endpoint call-order checks.aiopnsense/speedtest.py (1)
152-179: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing
"unavailable"case letsrun_speedtestinvoke the run endpoint even when the probe says the plugin is unavailable.The state check only handles
"missing",{"pending","transient"}, and"malformed". When_check_optional_get_endpointreturns"unavailable", none of the branches match, so execution falls through to the actual timed run request at Line 173 — contradicting the docstring's claim that the run is skipped "when the plugin endpoint is unavailable" and breaking the fail-closed contract other adapters (get_smart_result,get_unbound_blocklist_result) implement via a catch-all!= "available"check.🐛 Proposed fix
if optional_state == "missing": _LOGGER.debug("Speedtest not installed") return {} - if optional_state in {"pending", "transient"}: + if optional_state in {"unavailable", "pending", "transient"}: _LOGGER.debug("Speedtest temporarily unavailable") return {} if optional_state == "malformed": _LOGGER.debug("Speedtest probe returned malformed payload; proceeding with run request")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiopnsense/speedtest.py` around lines 152 - 179, Update run_speedtest to fail closed for every probe state other than "available", including "unavailable", by returning an empty mapping before invoking _safe_dict_get_with_timeout. Preserve the existing logging for recognized states and the malformed-payload behavior as appropriate, while ensuring only an available probe reaches SPEEDTEST_RUN_ENDPOINT.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@aiopnsense/client_base.py`:
- Around line 92-94: Move _dhcp_source_states_context from the client
initializer to module scope as a single shared ContextVar, preserving its name,
default=None, and type annotation; remove the per-instance creation while
keeping existing instance accesses unchanged.
In `@aiopnsense/dhcp.py`:
- Around line 118-121: Add debug logging to each malformed-detection branch in
the relevant DHCP category lookup methods, including the checks around
arp_table_info and the other referenced payload validations. Log the
source/category context and identify the invalid payload or missing/incorrect
rows before returning CategoryResult([], "malformed", False), matching the
existing Kea reservation malformed-rows logging style.
- Around line 101-126: The affected methods—get_arp_table_result,
get_dhcp_leases_result, _record_dhcp_source_state,
_unavailable_dhcp_source_state, and _aggregate_dhcp_source_states—need complete
Google-style docstrings with Args: and Returns: sections describing their
parameters and typed results. Update the Returns: documentation for
_get_kea_dhcp_leases, _get_dnsmasq_leases, and
_get_isc_dhcpv4_leases/_get_isc_dhcpv6_leases to document their provider-state
recording side effect, while preserving their existing behavior.
- Around line 361-364: Update the Kea lease-row validation around the address
and if_name type check to immediately continue when either value is invalid,
matching the ISC and dnsmasq handling. Keep setting malformed before skipping
the row, and ensure lease construction only runs for rows with valid string
values.
In `@aiopnsense/smart.py`:
- Around line 24-27: Expand the Google-style docstrings for get_smart_result in
aiopnsense/smart.py (lines 24-27) with a Returns section covering
CategoryResult[list[dict[str, Any]]] and its availability states; expand
get_unbound_blocklist_result in aiopnsense/unbound.py (lines 178-179) with
Returns documentation for CategoryResult[dict[str, Any]], including legacy and
extended shapes; add Args and Returns sections to _fetch_vnstat_for_result in
aiopnsense/vnstat.py (lines 58-61) documenting endpoint, expected_period, and
result states; and add a Returns section to get_vnstat_result in
aiopnsense/vnstat.py (lines 117-118) documenting aggregate CategoryResult
states.
In `@aiopnsense/vnstat.py`:
- Around line 174-182: Update the aggregate state selection in the category
result logic to include "unavailable" in the priority states checked from
daily_result and monthly_result. Ensure either sub-fetch being unavailable
returns CategoryResult with state "unavailable" and success false, while
preserving the existing priority ordering and available result for fully
available data.
In `@tests/test_category_result.py`:
- Around line 59-65: Move the DHCPMixin import from inside
test_dhcp_source_authority_ignores_only_confirmed_inapplicable_sources to the
module-level import section, keeping the test body focused on calling
DHCPMixin._aggregate_dhcp_source_states and asserting the expected result.
---
Outside diff comments:
In `@aiopnsense/dhcp.py`:
- Around line 527-589: Extract the duplicated ISC lease retrieval and parsing
logic from the v4 and v6 paths into one shared helper, parameterized with the
appropriate endpoint constants, interface and MAC field names, and
require_hardware_address behavior, following the existing _get_kea_dhcp_leases
pattern. Update both callers to use the helper while preserving CategoryResult
state recording, malformed-response handling, expiration filtering, and returned
lease fields.
In `@aiopnsense/speedtest.py`:
- Around line 152-179: Update run_speedtest to fail closed for every probe state
other than "available", including "unavailable", by returning an empty mapping
before invoking _safe_dict_get_with_timeout. Preserve the existing logging for
recognized states and the malformed-payload behavior as appropriate, while
ensuring only an available probe reaches SPEEDTEST_RUN_ENDPOINT.
In `@aiopnsense/unbound.py`:
- Around line 166-189: Update the malformed legacy branch in
get_unbound_blocklist_result so a falsy _get_unbound_blocklist_legacy() result
produces an empty data mapping while retaining the "malformed" status and false
availability metadata. Preserve the {"legacy": legacy} mapping for valid legacy
data and keep get_unbound_blocklist() returning a truthy mapping only when
blocklist data exists.
In `@tests/test_speedtest.py`:
- Around line 104-213: Update
test_get_speedtest_probes_showstat_before_fetching_optional_payload so the
showstat-available parameter supplies a non-empty payload that changes the
expected result, then assert that distinct showstat_available branches validate
the showstat-derived values versus the fallback values. Remove the redundant
identical assertions while preserving the endpoint call-order checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a17d8abd-d845-4de5-a647-bc64972ae11a
📒 Files selected for processing (23)
aiopnsense/__init__.pyaiopnsense/_typing.pyaiopnsense/client_base.pyaiopnsense/client_endpoint.pyaiopnsense/client_queue.pyaiopnsense/client_transport.pyaiopnsense/const.pyaiopnsense/dhcp.pyaiopnsense/nut.pyaiopnsense/smart.pyaiopnsense/speedtest.pyaiopnsense/unbound.pyaiopnsense/vnstat.pytests/test_category_result.pytests/test_client_endpoint.pytests/test_client_queue.pytests/test_client_transport.pytests/test_dhcp.pytests/test_nut.pytests/test_smart.pytests/test_speedtest.pytests/test_unbound.pytests/test_vnstat.py
| async def get_arp_table_result( | ||
| self, resolve_hostnames: bool = False | ||
| ) -> CategoryResult[list[dict[str, Any]]]: | ||
| """Return ARP rows with schema-aware endpoint authority metadata.""" | ||
| # [{'hostname': '?', 'ip-address': '<ip>', 'mac-address': '<mac>', 'interface': 'em0', 'expires': 1199, 'type': 'ethernet'}, ...] | ||
| resolve_flag = "yes" if resolve_hostnames else "no" | ||
| if not await self._is_get_endpoint_available(ARP_TABLE_ENDPOINT): | ||
| _LOGGER.debug("ARP endpoint unavailable") | ||
| return [] | ||
|
|
||
| arp_endpoint_resolve = f"{ARP_TABLE_ENDPOINT}?resolve={resolve_flag}" | ||
| arp_table_info = await self._safe_dict_get(arp_endpoint_resolve) | ||
| arp_table: list = arp_table_info.get("rows", []) | ||
| return arp_table | ||
| result = CategoryResult.coerce( | ||
| await self._check_optional_get_endpoint( | ||
| arp_endpoint_resolve, | ||
| cache_path=ARP_TABLE_ENDPOINT, | ||
| ) | ||
| ) | ||
| if result.state != "available": | ||
| _LOGGER.debug("ARP endpoint unavailable") | ||
| return CategoryResult([], result.state, False) | ||
| arp_table_info = result.data | ||
| if not isinstance(arp_table_info, MutableMapping): | ||
| return CategoryResult([], "malformed", False) | ||
| if "rows" not in arp_table_info or not isinstance(arp_table_info["rows"], list): | ||
| return CategoryResult([], "malformed", False) | ||
| rows = arp_table_info["rows"] | ||
| arp_table = [dict(row) for row in rows if isinstance(row, MutableMapping)] | ||
| if len(arp_table) != len(rows): | ||
| return CategoryResult(arp_table, "malformed", False) | ||
| return CategoryResult(arp_table, "available", True) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
New methods lack full Google-style docstrings (Args/Returns).
get_arp_table_result, get_dhcp_leases_result, _record_dhcp_source_state, _unavailable_dhcp_source_state, and _aggregate_dhcp_source_states all take parameters and/or return typed values but only have a single summary line — no Args:/Returns: sections, unlike other methods in this file (e.g. _normalize_lease_key_value, _copy_lease_identity_fields). Also, the unchanged Returns: docstrings for _get_kea_dhcp_leases/_get_dnsmasq_leases/_get_isc_dhcpv{4,6}_leases don't mention the new side effect of recording provider state via _record_dhcp_source_state.
As per coding guidelines, "Add or update Google Style docstrings for all files, classes, methods, private methods, and nested methods."
Also applies to: 146-149, 194-218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aiopnsense/dhcp.py` around lines 101 - 126, The affected
methods—get_arp_table_result, get_dhcp_leases_result, _record_dhcp_source_state,
_unavailable_dhcp_source_state, and _aggregate_dhcp_source_states—need complete
Google-style docstrings with Args: and Returns: sections describing their
parameters and typed results. Update the Returns: documentation for
_get_kea_dhcp_leases, _get_dnsmasq_leases, and
_get_isc_dhcpv4_leases/_get_isc_dhcpv6_leases to document their provider-state
recording side effect, while preserving their existing behavior.
Source: Coding guidelines
| if not isinstance(arp_table_info, MutableMapping): | ||
| return CategoryResult([], "malformed", False) | ||
| if "rows" not in arp_table_info or not isinstance(arp_table_info["rows"], list): | ||
| return CategoryResult([], "malformed", False) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Malformed-detection branches don't log a debug message.
Unlike the Kea reservation malformed branch (_LOGGER.debug("%s reservation lookup returned invalid rows payload", ...) at lines 337-339), the malformed checks here (and at lines 318-320, 466-468, 539-544, 542-544, 616-618) silently record state with no log line, making it harder to diagnose why a source was demoted to "malformed" in production. As per coding guidelines, "Add robust error handling and clear debug or info logs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aiopnsense/dhcp.py` around lines 118 - 121, Add debug logging to each
malformed-detection branch in the relevant DHCP category lookup methods,
including the checks around arp_table_info and the other referenced payload
validations. Log the source/category context and identify the invalid payload or
missing/incorrect rows before returning CategoryResult([], "malformed", False),
matching the existing Kea reservation malformed-rows logging style.
Source: Coding guidelines
| return (await self.get_smart_result()).data | ||
|
|
||
| async def get_smart_result(self) -> CategoryResult[list[dict[str, Any]]]: | ||
| """Return SMART device data with authoritative availability metadata.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
New *_result methods lack Google-style Returns/Args docstring sections. get_smart_result, get_unbound_blocklist_result, _fetch_vnstat_for_result, and get_vnstat_result are new public/internal methods returning typed CategoryResult objects with meaningful per-state semantics (available/malformed/missing/unavailable/etc.), but each has only a single-line docstring. As per coding guidelines, "Add or update Google Style docstrings for all files, classes, methods, private methods, and nested methods."
aiopnsense/smart.py#L24-L27: add aReturns:section toget_smart_resultdocumenting theCategoryResult[list[dict[str, Any]]]states.aiopnsense/unbound.py#L178-L179: add aReturns:section toget_unbound_blocklist_resultdocumenting theCategoryResult[dict[str, Any]]states, including the legacy vs. extended shape difference.aiopnsense/vnstat.py#L58-L61: addArgs:/Returns:sections to_fetch_vnstat_for_resultdocumentingendpoint,expected_period, and the resultingCategoryResultstates.aiopnsense/vnstat.py#L117-L118: add aReturns:section toget_vnstat_resultdocumenting the aggregateCategoryResultstates.
📍 Affects 3 files
aiopnsense/smart.py#L24-L27(this comment)aiopnsense/unbound.py#L178-L179aiopnsense/vnstat.py#L58-L61aiopnsense/vnstat.py#L117-L118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aiopnsense/smart.py` around lines 24 - 27, Expand the Google-style docstrings
for get_smart_result in aiopnsense/smart.py (lines 24-27) with a Returns section
covering CategoryResult[list[dict[str, Any]]] and its availability states;
expand get_unbound_blocklist_result in aiopnsense/unbound.py (lines 178-179)
with Returns documentation for CategoryResult[dict[str, Any]], including legacy
and extended shapes; add Args and Returns sections to _fetch_vnstat_for_result
in aiopnsense/vnstat.py (lines 58-61) documenting endpoint, expected_period, and
result states; and add a Returns section to get_vnstat_result in
aiopnsense/vnstat.py (lines 117-118) documenting aggregate CategoryResult
states.
Source: Coding guidelines
| data = {"interfaces": interface_data, "interface_count": len(interface_names)} | ||
| non_available = [ | ||
| item.state for item in (daily_result, monthly_result) if item.state != "available" | ||
| ] | ||
| if non_available: | ||
| for state in ("pending", "transient", "malformed", "missing"): | ||
| if state in non_available: | ||
| return CategoryResult(data, state, False) | ||
| return CategoryResult(data, "available", True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
"unavailable" daily/monthly state silently reported as "available".
The priority list ("pending", "transient", "malformed", "missing") omits "unavailable". If daily_result.state or monthly_result.state is "unavailable", it lands in non_available but the loop never matches it, so the method falls through to return CategoryResult(data, "available", True) — reporting the aggregate result as fully available/authoritative despite a known-unavailable sub-fetch.
🐛 Proposed fix
if non_available:
- for state in ("pending", "transient", "malformed", "missing"):
+ for state in ("pending", "transient", "malformed", "missing", "unavailable"):
if state in non_available:
return CategoryResult(data, state, False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data = {"interfaces": interface_data, "interface_count": len(interface_names)} | |
| non_available = [ | |
| item.state for item in (daily_result, monthly_result) if item.state != "available" | |
| ] | |
| if non_available: | |
| for state in ("pending", "transient", "malformed", "missing"): | |
| if state in non_available: | |
| return CategoryResult(data, state, False) | |
| return CategoryResult(data, "available", True) | |
| data = {"interfaces": interface_data, "interface_count": len(interface_names)} | |
| non_available = [ | |
| item.state for item in (daily_result, monthly_result) if item.state != "available" | |
| ] | |
| if non_available: | |
| for state in ("pending", "transient", "malformed", "missing", "unavailable"): | |
| if state in non_available: | |
| return CategoryResult(data, state, False) | |
| return CategoryResult(data, "available", True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aiopnsense/vnstat.py` around lines 174 - 182, Update the aggregate state
selection in the category result logic to include "unavailable" in the priority
states checked from daily_result and monthly_result. Ensure either sub-fetch
being unavailable returns CategoryResult with state "unavailable" and success
false, while preserving the existing priority ordering and available result for
fully available data.
| def test_dhcp_source_authority_ignores_only_confirmed_inapplicable_sources( | ||
| states: list[CategoryState], expected: tuple[CategoryState, bool] | ||
| ) -> None: | ||
| """Confirmed missing providers are inapplicable; uncertain providers are not.""" | ||
| from aiopnsense.dhcp import DHCPMixin | ||
|
|
||
| assert DHCPMixin._aggregate_dhcp_source_states(states) == expected |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Local import inside test function.
from aiopnsense.dhcp import DHCPMixin is imported inside the test body rather than at the top of the file with the other imports. As per coding guidelines, "keep imports at the top of Python files."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_category_result.py` around lines 59 - 65, Move the DHCPMixin
import from inside
test_dhcp_source_authority_ignores_only_confirmed_inapplicable_sources to the
module-level import section, keeping the test body focused on calling
DHCPMixin._aggregate_dhcp_source_states and asserting the expected result.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
aiopnsense/dhcp.py (2)
324-334: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSeveral new/changed methods dropped Args/Returns sections from their docstrings.
_get_kea_dhcp_leases_result(Lines 356-371) shows the expected full Google-style format, but the sites below were reduced to a single summary line despite taking parameters or returning non-trivial types.
aiopnsense/dhcp.py#L324-L334:_get_kea_dhcp_leasestakes six parameters (lease_endpoint,service_name,reservation_endpoint,reservation_camelcase_endpoint,require_hardware_address,dynamic_when_reservation_lookup_unavailable) and returnslist— none documented.aiopnsense/dhcp.py#L100-L103:get_arp_table_resulttakesresolve_hostnamesand returnsCategoryResult[list[dict[str, Any]]]— add Args/Returns.aiopnsense/dhcp.py#L145-L148:get_dhcp_leases_resulttakesopnsense_tzand returnsCategoryResult[dict[str, Any]]— add Args/Returns.aiopnsense/dhcp.py#L193-L197:_record_dhcp_source_statetakesstate— add an Args section.aiopnsense/dhcp.py#L199-L203:_aggregate_dhcp_source_statestakessource_statesand returns atuple[CategoryState, bool]— add Args/Returns.As per path instructions for
**/*.py: "Add or update Google Style docstrings for all files, classes, methods, private methods, and nested methods."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiopnsense/dhcp.py` around lines 324 - 334, Complete the Google-style docstrings for _get_kea_dhcp_leases, get_arp_table_result, get_dhcp_leases_result, _record_dhcp_source_state, and _aggregate_dhcp_source_states in aiopnsense/dhcp.py at lines 324-334, 100-103, 145-148, 193-197, and 199-203. Add Args entries for each parameter and Returns entries for the non-void methods, using the stated types and descriptions consistent with _get_kea_dhcp_leases_result; _record_dhcp_source_state requires Args only.Source: Coding guidelines
347-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHigh complexity flagged by Ruff (PLR0912/PLR0915).
_get_kea_dhcp_leases_resulttrips both "too many branches" (29>12) and "too many statements" (76>50). This is also the function containing the missing-continuebug flagged above — a concrete sign the branching validation chain (endpoint check → response shape → reservation shape → per-row shape) would benefit from extraction into smaller, independently testable helpers (e.g., a_validate_kea_reservation_rowsand_validate_kea_lease_rowhelper).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiopnsense/dhcp.py` around lines 347 - 480, Reduce the complexity of _get_kea_dhcp_leases_result by extracting reservation-row validation and lease-row normalization/validation into focused helpers, such as _validate_kea_reservation_rows and _validate_kea_lease_row. Keep endpoint-state handling and result aggregation in the parent method, preserve all existing lease classification and malformed-state behavior, and ensure invalid rows are explicitly skipped before field access or lease creation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@aiopnsense/_typing.py`:
- Line 8: Update the imports in _typing.py to import Iterator from
collections.abc alongside the existing collections.abc symbols, and remove
Iterator from the typing import. Keep the __iter__ annotation using the same
Iterator symbol.
In `@aiopnsense/dhcp.py`:
- Around line 429-437: Update the malformed lease validation in the relevant
DHCP lease parsing function so that after setting malformed for an invalid or
missing address or if_name, it immediately continues to the next lease row.
Match the existing behavior in _get_isc_dhcpv4_leases, _get_isc_dhcpv6_leases,
and _get_dnsmasq_leases_result, ensuring malformed entries are never constructed
or appended.
In `@aiopnsense/unbound.py`:
- Around line 187-189: Update the malformed legacy branch in
get_unbound_blocklist to return an empty mapping directly, matching the
documented malformed-response contract and the extended malformed path; preserve
the existing available legacy result unchanged.
In `@tests/test_category_result.py`:
- Around line 221-229: Update the nested helpers in
tests/test_category_result.py: annotate source with a Callable[...,
Awaitable[list[dict]]] return type and annotate provider’s **_kwargs as object;
also annotate **_kwargs as object in first_provider and missing_provider. Add
any required typing imports while preserving the existing helper behavior.
- Line 63: Move the DHCPMixin import from inside the test function to the
module-level import section at the top of tests/test_category_result.py,
preserving the existing test behavior and comments.
In `@tests/test_client_endpoint.py`:
- Line 885: Update the get stub’s unused **_kwargs annotation to object instead
of Any, matching the other session stubs and avoiding the unnecessary ANN401
violation; remove any now-unused Any import if applicable.
In `@tests/test_speedtest.py`:
- Around line 205-212: Update the assertions around the showstat_available
branch so it validates the showstat-specific result: assert the expected
average.* values when showstat_available is true, and assert those fields are
empty or None otherwise. Keep the existing last.* assertions for showlog data,
but ensure the three parametrized showstat cases exercise distinct expectations.
---
Outside diff comments:
In `@aiopnsense/dhcp.py`:
- Around line 324-334: Complete the Google-style docstrings for
_get_kea_dhcp_leases, get_arp_table_result, get_dhcp_leases_result,
_record_dhcp_source_state, and _aggregate_dhcp_source_states in
aiopnsense/dhcp.py at lines 324-334, 100-103, 145-148, 193-197, and 199-203. Add
Args entries for each parameter and Returns entries for the non-void methods,
using the stated types and descriptions consistent with
_get_kea_dhcp_leases_result; _record_dhcp_source_state requires Args only.
- Around line 347-480: Reduce the complexity of _get_kea_dhcp_leases_result by
extracting reservation-row validation and lease-row normalization/validation
into focused helpers, such as _validate_kea_reservation_rows and
_validate_kea_lease_row. Keep endpoint-state handling and result aggregation in
the parent method, preserve all existing lease classification and
malformed-state behavior, and ensure invalid rows are explicitly skipped before
field access or lease creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be14662b-056d-460e-9d6c-71ae1cfd5cf1
📒 Files selected for processing (24)
aiopnsense/__init__.pyaiopnsense/_typing.pyaiopnsense/client_base.pyaiopnsense/client_endpoint.pyaiopnsense/client_queue.pyaiopnsense/client_transport.pyaiopnsense/const.pyaiopnsense/dhcp.pyaiopnsense/nut.pyaiopnsense/smart.pyaiopnsense/speedtest.pyaiopnsense/unbound.pyaiopnsense/vnstat.pytests/test_category_result.pytests/test_client_endpoint.pytests/test_client_queue.pytests/test_client_transport.pytests/test_dhcp.pytests/test_dhcp_optional_results.pytests/test_nut.pytests/test_smart.pytests/test_speedtest.pytests/test_unbound.pytests/test_vnstat.py
| from dataclasses import dataclass | ||
| from datetime import tzinfo | ||
| from typing import Any, Protocol | ||
| from typing import Any, Iterator, Literal, Protocol |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Import Iterator from collections.abc.
typing.Iterator is deprecated; on this 3.14 target prefer collections.abc.Iterator (used at Line 40 for __iter__).
♻️ Suggested import change
-from typing import Any, Iterator, Literal, Protocol
+from collections.abc import AsyncGenerator, Iterator, MutableMapping
+from typing import Any, Literal, Protocol(merge Iterator into the existing collections.abc import on Line 4 and drop it from typing.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import Any, Iterator, Literal, Protocol | |
| from collections.abc import AsyncGenerator, Iterator, MutableMapping | |
| from typing import Any, Literal, Protocol |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 8-8: Import from collections.abc instead: Iterator
Import from collections.abc
(UP035)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@aiopnsense/_typing.py` at line 8, Update the imports in _typing.py to import
Iterator from collections.abc alongside the existing collections.abc symbols,
and remove Iterator from the typing import. Keep the __iter__ annotation using
the same Iterator symbol.
Source: Linters/SAST tools
| states: list[CategoryState], expected: tuple[CategoryState, bool] | ||
| ) -> None: | ||
| """Confirmed missing providers are inapplicable; uncertain providers are not.""" | ||
| from aiopnsense.dhcp import DHCPMixin |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Move the DHCPMixin import to the top of the file.
Local import inside the test function; the repository guideline requires imports at the top of Python files, and nothing here suggests a circular-import necessity for a test module.
♻️ Proposed fix
+from aiopnsense.dhcp import DHCPMixin
from aiopnsense import CategoryResult, CategoryState, OPNsenseClient
@@
def test_dhcp_source_authority_ignores_only_confirmed_inapplicable_sources(
states: list[CategoryState], expected: tuple[CategoryState, bool]
) -> None:
"""Confirmed missing providers are inapplicable; uncertain providers are not."""
- from aiopnsense.dhcp import DHCPMixin
-
assert DHCPMixin._aggregate_dhcp_source_states(states) == expectedAs per coding guidelines, "Preserve existing comments and keep imports at the top of Python files."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from aiopnsense.dhcp import DHCPMixin | |
| from aiopnsense.dhcp import DHCPMixin | |
| from aiopnsense import CategoryResult, CategoryState, OPNsenseClient | |
| def test_dhcp_source_authority_ignores_only_confirmed_inapplicable_sources( | |
| states: list[CategoryState], expected: tuple[CategoryState, bool] | |
| ) -> None: | |
| """Confirmed missing providers are inapplicable; uncertain providers are not.""" | |
| assert DHCPMixin._aggregate_dhcp_source_states(states) == expected |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_category_result.py` at line 63, Move the DHCPMixin import from
inside the test function to the module-level import section at the top of
tests/test_category_result.py, preserving the existing test behavior and
comments.
Source: Coding guidelines
| def source(state: CategoryState, leases: list[dict]): | ||
| """Build a synthetic provider that records state and returns normalized rows.""" | ||
|
|
||
| async def provider(**_kwargs) -> list[dict]: | ||
| """Record the provider state for this collection pass.""" | ||
| client._record_dhcp_source_state(state) | ||
| return leases | ||
|
|
||
| return provider |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Untyped test-helper closures across this file. Several nested async helper functions omit type annotations required by the repo's Python guideline ("Add type annotations, including return types, to all functions and classes"); one annotation pass fixes all sites.
tests/test_category_result.py#L221-L229: add a return type tosource(e.g.Callable[..., Awaitable[list[dict]]]) and annotateprovider's**_kwargs: object.tests/test_category_result.py#L321-L321,334-334: annotate**_kwargs: objecton bothfirst_providerandmissing_provider.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 221-221: Missing return type annotation for private function source
(ANN202)
[warning] 224-224: Missing type annotation for **_kwargs
(ANN003)
📍 Affects 1 file
tests/test_category_result.py#L221-L229(this comment)tests/test_category_result.py#L321-L321
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_category_result.py` around lines 221 - 229, Update the nested
helpers in tests/test_category_result.py: annotate source with a Callable[...,
Awaitable[list[dict]]] return type and annotate provider’s **_kwargs as object;
also annotate **_kwargs as object in first_provider and missing_provider. Add
any required typing imports while preserving the existing helper behavior.
Sources: Coding guidelines, Linters/SAST tools
| client, session = make_mock_session_client(make_client) | ||
| requested_urls: list[str] = [] | ||
|
|
||
| def get(url: str, **_kwargs: Any) -> FakeResponse: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use object instead of Any for the stub's **_kwargs.
The other new session stubs in this file type **_kwargs as object; this one uses Any, which ruff flags (ANN401) and is unnecessary here since _kwargs is unused.
🧹 Proposed fix
- def get(url: str, **_kwargs: Any) -> FakeResponse:
+ def get(url: str, **_kwargs: object) -> FakeResponse:As per path instructions: "minimize cast and Any unless required at a test boundary."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get(url: str, **_kwargs: Any) -> FakeResponse: | |
| def get(url: str, **_kwargs: object) -> FakeResponse: |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 885-885: Dynamically typed expressions (typing.Any) are disallowed in **_kwargs
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_client_endpoint.py` at line 885, Update the get stub’s unused
**_kwargs annotation to object instead of Any, matching the other session stubs
and avoiding the unnecessary ANN401 violation; remove any now-unused Any import
if applicable.
Sources: Path instructions, Linters/SAST tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 048fb08ea6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| response.status, | ||
| response.reason, | ||
| ) | ||
| except (aiohttp.ClientError, TimeoutError) as e: |
There was a problem hiding this comment.
When a forced availability refresh hits this TimeoutError/aiohttp.ClientError branch after the endpoint already has a fresh cached "available" entry, the branch returns False but leaves the old cache entry and timestamp intact. The next non-forced _is_get_endpoint_available/_is_post_endpoint_available call will therefore return the stale cached success until the positive TTL expires, even though the refresh just observed the endpoint failing; clear the cached observation before returning or raising from this exception path.
Useful? React with 👍 / 👎.
| async def _get_kea_interfaces(self) -> dict[str, Any]: | ||
| """Return the data from the status-aware Kea interface lookup.""" | ||
| result = await self._get_kea_interfaces_result() | ||
| self._record_dhcp_source_state(result.state) |
There was a problem hiding this comment.
When all real lease endpoints are confirmed missing but /api/kea/dhcpv4/get is reachable and reports Kea disabled, _get_kea_interfaces_result() returns CategoryResult({}, "available", True) and this line records that as a DHCP source state. _aggregate_dhcp_source_states() then sees an available state and reports the whole DHCP lease result as authoritative, even though no lease provider returned data; keep this interface metadata state separate from lease-source availability or ignore its available result for aggregation.
Useful? React with 👍 / 👎.
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
1 participant
Summary
Adds status-aware reconciliation for optional OPNsense API capabilities so active plugin requests refresh endpoint availability, real 404 responses invalidate stale positives, and confirmed absences recover on a short negative-cache window.
What Changed
showlog/showstat, NUT, vnStat, Unbound DNSBL, and SMART callers to the shared reconciliation contract.Why
The previous six-hour availability cache could leave an optional endpoint marked present after its plugin was removed, causing repeated 404 requests. The same cache could also delay detection of a newly installed plugin for up to six hours.
This design uses the real payload request as the positive observation, avoiding a separate availability probe on every update. A 404 invalidates the exact cached observation, healthy repeated absence creates a five-minute negative entry, and transport or router-wide failures leave capability state unpoisoned.
Summary by CodeRabbit
Greptile Summary
This PR replaces the previous 6-hour flat availability cache with a two-phase, state-aware reconciliation system for optional OPNsense plugin endpoints. Real payload requests act as positive observations, real 404s go through a "pending → confirmed-negative" confirmation gate guarded by a core-firmware health check, and confirmed absences use a short 5-minute negative TTL so recovered plugins are detected quickly.
CategoryResult[T]dataclass (_typing.py) enforces anauthoritative ↔ state=="available"invariant at construction time and provides backward-compatible__iter__/__eq__for the legacy tuple callers still in flight._check_optional_endpoint(client_endpoint.py) is the core primitive: per-endpoint async locks, double-checked caching, two-phase 404 confirmation, and a firmware health guard to prevent router-wide outages from poisoning individual plugin state.get_*_result()methods returningCategoryResultwhile preserving the existingget_*()convenience wrappers;run_speedtestcorrectly distinguishes "malformed" (proceeds) from "missing"/"pending"/"transient" (blocks), closing the regression noted in a prior review thread.Confidence Score: 5/5
Safe to merge — the reconciliation logic is internally consistent, all state transitions are properly serialised under per-endpoint locks, and the firmware health guard prevents router-wide outages from poisoning individual plugin availability state.
The two-phase 404 confirmation, TTL handling, and backward-compat shims are all well-reasoned and the test suite covers removal, transient outages, negative-TTL expiry, recovery, malformed payloads, concurrency, and non-404 failures. The two observations filed are minor design nits that do not affect current runtime correctness.
No files require special attention;
client_endpoint.pycarries the most complex logic but the lock and state-machine invariants hold up under scrutiny.Important Files Changed
is_payload_specific_smart_infohardcode is the only coupling concern.CategoryResult[T]dataclass with strictauthoritative ↔ state=="available"invariant enforced in__post_init__; adds backward-compat__iter__and__eq__for legacy tuple migration; clean and well-structured._do_optional_get/_do_optional_postunified via_do_optional_request; correctly classifies 200 (available), JSON-decode failure (malformed), 404 (missing), 403/other (transient). Python 3.14+except TypeError, ValueError:syntax is valid per PEP 758._get_optional/_post_optionalqueue wrappers and dispatchesoptional_get/optional_postin_process_queue; Python 3.14+ comma-separated exception syntax is valid; no issues.run_speedtestcorrectly distinguishes "malformed" (proceeds with run) from "missing"/"pending"/"transient" (blocks run), addressing the previously noted regression.get_smart_resultandget_smart_info_resultcallers correctly passcache_path=SMART_SERVICE_LIST_ENDPOINTfor the detail endpoint; per-device info bypass of "missing" cache is intentional.get_nut_ups_status_resultwith thorough payload validation distinguishing empty-UPS (available) from malformed status/response formats; no issues.get_vnstat_resultpropagates partial hourly data with a degraded state when daily/monthly sub-endpoints fail; state priority ordering is intentional.get_unbound_blocklist_resultwraps both legacy and extended DNSBL paths inCategoryResult; legacy path returns "malformed" on empty settings rather than silently succeeding; clean.Reviews (5): Last reviewed commit: "Address current endpoint review findings" | Re-trigger Greptile