Alternative v6 XM approach #122
Conversation
WalkthroughAdds AirOS v6 XM login flow with cookie activation and api_version tracking; switches auth to raw cookie headers; promotes derived_data to a classmethod and adds fw_major/mac helpers; extends data models for v6 (CPU counters, numeric frequency, polling, antenna_gain, ieeemode); adds v6 fixtures, tests, bump version, and ignores Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant AirOS as AirOS.login()
participant V8 as v8 (_login_url)
participant XM as v6 (_v6_xm_login_url)
participant Form as v6 (_v6_form_url)
Client->>AirOS: login(credentials)
AirOS->>V8: POST credentials (attempt v8)
alt v8 accepted
V8-->>AirOS: auth cookie
AirOS->>Client: connected=true, api_version=8
else v8 404 / not found
AirOS->>XM: GET /login.cgi (fetch cookie)
XM-->>AirOS: Set-Cookie
AirOS->>XM: POST /login.cgi (credentials) -> expect 302
XM-->>AirOS: 302 redirect
AirOS->>Form: GET /index.cgi (activate session)
Form-->>AirOS: activation OK (session cookie active)
AirOS->>Client: connected=true, api_version=6
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
- Coverage 89.57% 87.07% -2.50%
==========================================
Files 8 8
Lines 681 743 +62
==========================================
+ Hits 610 647 +37
- Misses 71 96 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
airos/base.py (2)
264-274: Handle non-JSON login responses to avoid false failures.Login endpoints may return 204/HTML. Skip JSON parsing for login URLs.
- response_text = await response.text() - _LOGGER.debug("Successfully fetched JSON from %s", url) - - # If this is the login request, we need to store the new auth data - if url in self._login_urls: - self._store_auth_data(response) - self.connected = True - - return json.loads(response_text) + _LOGGER.debug("Request to %s succeeded", url) + # Login endpoints: store auth and return immediately without JSON parsing + if url in self._login_urls: + self._store_auth_data(response) + self.connected = True + return {} + # Other endpoints: parse JSON as usual + response_text = await response.text() + return json.loads(response_text)Also applies to: 268-272
383-391: Bug: Inconsistent body vs Content-Type in update_check(force=True).You're sending JSON with form Content-Type. Align with JSON for both branches.
- if force: - return await self._request_json( - "POST", - self._update_check_url, - json_data={"force": True}, - authenticated=True, - ct_form=True, - ) + if force: + return await self._request_json( + "POST", + self._update_check_url, + json_data={"force": True}, + authenticated=True, + ct_json=True, + )Also applies to: 396-397
🧹 Nitpick comments (4)
.gitignore (1)
15-15: Anchor ignore to root (or use local exclude) to avoid unintended matches.Use "/test.py" if you only mean the repo‑root file, or prefer .git/info/exclude locally to avoid committing personal helpers.
Is this meant to ignore only the root-level test.py?
airos/base.py (3)
58-59: Minor: avoid magic number for API versions.Consider an Enum or constants (e.g., API_V8 = 8) for readability.
95-95: Use an immutable container for login URLs.Tuple (or set) communicates intent and avoids accidental mutation.
- self._login_urls = [self._login_url, self._v6_xm_login_url] + self._login_urls = (self._login_url, self._v6_xm_login_url)
293-342: _login_v6 flow LGTM; consider reuse and headers parity.Optional: call _store_auth_data on the initial GET to reuse parsing; ensure Referer/Origin are consistent across calls.
Do you want me to refactor _login_v6 to reuse _store_auth_data after we fix the cookie handling?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)airos/base.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airos/base.py (1)
airos/exceptions.py (3)
AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSUrlNotFoundError(44-45)
🔇 Additional comments (3)
airos/base.py (3)
82-85: v6 endpoints look reasonable.LGTM; the leading slash on _v6_form_url matches f"{self.base_url}{...}" usage.
253-256: Pre-login request gating is good.Prevents accidental calls before authentication. LGTM.
347-349: Graceful fallback to v6 on 404 looks good.Clear control flow; preserves 8.x first, then v6.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
airos/base.py (2)
213-214: Past review concern has been addressed; remove commented code.The previous review flagged a critical bug about the "AIROS_" prefix being stripped. This has been resolved—line 229 now stores the full cookie as
f"{morsel.key}={morsel.value}", preserving the "AIROS_" prefix. The current implementation is correct.Remove the commented-out line for cleanliness:
if self._auth_cookie: # pragma: no cover - # headers["Cookie"] = f"AIROS_{self._auth_cookie}" headers["Cookie"] = self._auth_cookie
228-229: Past review concern has been addressed; remove commented code.The cookie is now correctly stored with the full key name (including "AIROS_" prefix), addressing the previous review's critical concern.
Remove the commented-out line:
if key.startswith("AIROS_"): - # self._auth_cookie = morsel.key[6:] + "=" + morsel.value self._auth_cookie = f"{morsel.key}={morsel.value}" break
🧹 Nitpick comments (3)
airos/base.py (3)
302-304: Optional: Simplify cookie retrieval.The cookie retrieval logic is correct but could be more readable.
Consider this slightly clearer approach:
- session_cookie = next( - (c for n, c in response.cookies.items() if n.startswith("AIROS")), None - ) + airos_cookies = [c for n, c in response.cookies.items() if n.startswith("AIROS")] + session_cookie = airos_cookies[0] if airos_cookies else None
339-340: Consider more robust session activation check.Checking for "login.cgi" in the URL string works but is fragile—it could match query parameters or other unintended parts of the URL.
Consider a more explicit check:
- if "login.cgi" in str(response.url): + if response.url.path.endswith("login.cgi"): raise AirOSConnectionAuthenticationError("Session activation failed.")
344-355: Optional: Remove unnecessary return statement.The
returnon line 354 is unnecessary since the function returnsNoneimplicitly and it's at the end of the function.Simplify:
async def login(self) -> None: """Login to AirOS device.""" payload = {"username": self.username, "password": self.password} try: await self._request_json("POST", self._login_url, json_data=payload) except AirOSUrlNotFoundError: await self._login_v6() except AirOSConnectionSetupError as err: raise AirOSConnectionSetupError("Failed to login to AirOS device") from err - else: - returnOr if you prefer explicit returns, keep it but remove the
else:async def login(self) -> None: """Login to AirOS device.""" payload = {"username": self.username, "password": self.password} try: await self._request_json("POST", self._login_url, json_data=payload) except AirOSUrlNotFoundError: await self._login_v6() except AirOSConnectionSetupError as err: raise AirOSConnectionSetupError("Failed to login to AirOS device") from err - else: - return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airos/base.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airos/base.py (2)
tests/conftest.py (1)
base_url(18-20)airos/exceptions.py (3)
AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSUrlNotFoundError(44-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (4)
airos/base.py (4)
58-59: LGTM: Sensible default for API version.Setting
api_versionto 8 by default is appropriate, as it will be updated to 6 in the_login_v6()flow when needed.
95-95: LGTM: Clean centralization of login URLs.The
_login_urlslist provides a clean way to check if a URL is a login endpoint, improving maintainability.
254-254: LGTM: Correct membership checks for login URLs.The use of
self._login_urlslist for membership checks properly handles both v8 and v6 login endpoints.Also applies to: 270-270
294-343: Good implementation of v6 XM login flow.The three-step v6 login process is well-structured:
- Retrieve session cookie from login endpoint
- POST credentials with 302 redirect expectation
- Activate session via form URL
Error handling appropriately distinguishes connection errors from authentication failures, and the cookie handling is consistent with the v8 path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_stations6.py (1)
77-101: Consider adding explicit__aexit__setup for async context managers.While
MagicMockmay auto-mock the missing__aexit__, explicitly setting it makes the test more robust and clear:Apply this pattern to each response mock:
get_login_response = MagicMock() get_login_response.__aenter__.return_value = get_login_response +get_login_response.__aexit__.return_value = AsyncMock(return_value=None)Repeat for
post_login_responseandget_index_response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_stations6.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_stations6.py (2)
airos/base.py (2)
status(356-380)_login_v6(294-342)airos/airos6.py (1)
AirOS6(17-82)
🪛 Gitleaks (8.28.0)
tests/test_stations6.py
[high] 116-116: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (3)
tests/test_stations6.py (3)
7-7: LGTM!The addition of
MagicMockto the imports is appropriate for the new test function.
104-109: Verify host parameter format and use_ssl consistency.The
hostparameter includes the scheme"http://192.168.1.3", butuse_ssldefaults toTrue, which suggests HTTPS. Ensure this is intentional and that the base class correctly handles a host with an explicit scheme, or adjust the parameters for consistency:airos6_device = AirOS6( - host="http://192.168.1.3", + host="192.168.1.3", username="ubnt", password="ubnt", session=session, + use_ssl=False, )
111-117: LGTM! Test assertions align with the v6 login flow.The test correctly validates:
- Connection state after login
- API version assignment
- Auth cookie format
- Expected number of HTTP requests
Note: The static analysis hint flagging line 116 is a false positive—this is a test assertion checking the cookie value, not an exposed secret.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airos/base.py (1)
382-399: Content-Type/json mismatch in update_check(force=True).You send JSON (
json_data={"force": True}) but set form Content-Type (ct_form=True). Usect_json=Truefor consistency and correctness.- if force: - return await self._request_json( - "POST", - self._update_check_url, - json_data={"force": True}, - authenticated=True, - ct_form=True, - ) + if force: + return await self._request_json( + "POST", + self._update_check_url, + json_data={"force": True}, + authenticated=True, + ct_json=True, + )
♻️ Duplicate comments (3)
airos/base.py (3)
212-215: Cookie handling fix retained full cookie name.Keeping the full
AIROS_*cookie string in the header resolves the v6/v8 interoperability issue. LGTM.
222-230: Parsing Set-Cookie for AIROS_ and storing full pair is correct.*This matches device behavior and prior feedback. LGTM.
82-85: Minor: endpoint URL consistency.
_v6_form_urlis a path while others are absolute. It works (you prefix at use), but consider normalizing for consistency or add a clarifying comment.
🧹 Nitpick comments (4)
airos/model_map.py (1)
99-145: Good normalization; consider clarifying match semantics.The
strip()fix is spot on. The variablebest_match_is_prefixis misleading since you’re usingendswith(); consider renaming to avoid confusion and add a brief comment with an example (“devmodel is a suffix of the catalog name: 'LiteBeam 5AC' ⟶ 'airMAX LiteBeam 5AC'”).tests/test_stations6.py (2)
67-71: Remove unused cookie setup.This snippet isn’t used by assertions; drop it to reduce noise.
- cookie = SimpleCookie() - cookie["session_id"] = "test-cookie" - cookie["AIROS_TOKEN"] = "abc123"
118-118: Gitleaks false positive: replace secret-looking test token.The hardcoded
"AIROS_ABC123=xyz789"can trigger secret scanners. The diff above switches to"AIROS_TEST=nonsecret". Alternatively, build the string from variables to avoid static patterns.fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json (1)
24-24: Consider adjustingcpuloadto a more realistic baseline value.The fixture specifies
cpuload: 100.0, which represents 100% CPU utilization. For a typical test fixture representing normal device state, a lower baseline value (e.g., 15–40) would be more realistic and representative. If this represents a specific high-load test scenario, consider documenting it or creating a separate fixture variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)airos/base.py(9 hunks)airos/model_map.py(3 hunks)fixtures/airos_NanoStation_M5_sta_v6.3.16.json(1 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json(1 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json(1 hunks)fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_ap.json(1 hunks)fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_sta.json(1 hunks)pyproject.toml(1 hunks)tests/test_stations6.py(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json
- fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_ap.json
- fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_sta.json
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_stations6.py (2)
airos/base.py (2)
status(356-380)_login_v6(294-342)airos/airos6.py (1)
AirOS6(17-82)
airos/base.py (1)
airos/exceptions.py (3)
AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSUrlNotFoundError(44-45)
🪛 Gitleaks (8.28.0)
tests/test_stations6.py
[high] 118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (4)
pyproject.toml (1)
7-7: Confirm pre-release intent and automation.Version "0.5.7a9" is a pre-release (PEP 440). Ensure your publish/CI and consumers expect pre-releases; otherwise, pinning/upgrades may skip this version.
airos/model_map.py (1)
81-88: Public mappings look good.Splitting SITE_MODELS and MANUAL_MODELS and merging into MODELS improves clarity and extensibility. No issues.
fixtures/airos_NanoStation_M5_sta_v6.3.16.json (1)
13-13: Fixture SKU correction matches mapper behavior.
"sku": "NSM5"aligns with the normalized"NanoStation M5 "->NSM5mapping. Good.fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json (1)
5-15: No action required - this is not a data inconsistency.The fixture is correct.
derived.moderepresents the device's network topology configuration (point-to-point wireless link), whilewireless.moderepresents its operating mode (access point). These are two distinct concepts. Every similar fixture in the codebase (includingap-ptp,sta-ptpvariants) consistently usesderived.mode: "point_to_point"withptp: true, confirming this is the intended design pattern for devices operating in PTP topology.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airos/data.py (1)
607-609: Consider more robust frequency parsing.The current parsing assumes a space between the number and "MHz" (e.g.,
"5180 MHz"). If the format varies (e.g.,"5180MHz"),split()[0]might not work as expected.Consider a more flexible approach:
freq = d.get("frequency") if isinstance(freq, str) and "MHz" in freq: - d["frequency"] = int(freq.split()[0]) + d["frequency"] = int(freq.replace("MHz", "").strip())This handles both
"5180 MHz"and"5180MHz"formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)airos/data.py(5 hunks)fixtures/airos_NanoStation_M5_sta_v6.3.16.json(4 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json(1 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~10-~10: Ensure spelling is correct
Context: ...XW already was successful) - Calculated cpuload on v6 if values available (to prevent r...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (5)
fixtures/airos_NanoStation_M5_sta_v6.3.16.json (1)
1-165: LGTM! Fixture aligns with v6 data model.The fixture data correctly reflects the v6 data structure changes:
- CPU metrics (cpubusy, cputotal, cpuload) align with Host6 model
- Numeric frequency (5180) and rates (216, 270) match Wireless6 type changes
- Throughput object matches the derived field structure
airos/data.py (3)
249-263: LGTM! Safe cpuload calculation with proper guards.The pre-deserialize hook correctly:
- Validates both cpubusy and cputotal are numeric
- Prevents division by zero with
cputotal > 0check- Computes percentage and rounds to 2 decimal places
574-596: LGTM! Type changes align with v6 numeric data format.The field type updates (frequency, txrate, rxrate to int, and throughput addition) correctly reflect the v6 data structure. The pre-deserialize hook handles conversion from string formats.
611-618: LGTM! Throughput derivation handles type conversions safely.The logic correctly:
- Retrieves rxrate and txrate from the data
- Uses
int(float())conversion to handle numeric strings and floats- Defaults to 0 when values are missing or falsy
- Creates properly structured throughput dictionary
CHANGELOG.md (1)
5-12: LGTM! Changelog accurately documents v6 enhancements.The version 0.5.7 entry clearly describes:
- v6 XM model login support
- CPU load calculation improvements
- Frequency and rate handling for v6 firmware
Note: The static analysis warning about "cpuload" spelling is a false positive—it's valid technical terminology for CPU load metrics.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airos/data.py (1)
43-103: MAC redaction can yield invalid MAC; redact last two octets explicitly.Using the last 5 characters is brittle. Split by octets to retain “AA:BB”.
- if isinstance(v, str) and (is_mac_address(v) or is_mac_address_mask(v)): - # Redact only the last part of a MAC address to a dummy value - redacted_d[k] = "00:11:22:33:" + v.replace("-", ":").upper()[-5:] + if isinstance(v, str) and (is_mac_address(v) or is_mac_address_mask(v)): + # Keep last two octets; normalize separators + norm = v.replace("-", ":").upper() + parts = norm.split(":") + tail = ":".join(parts[-2:]) if len(parts) >= 2 else "00:00" + redacted_d[k] = f"00:11:22:33:{tail}"
🧹 Nitpick comments (4)
airos/helpers.py (2)
35-37: Docstring and naming drift.Function returns fw_major, hostname, and mac — not just “major firmware version.” Update docstring for accuracy.
- """Connect to a device and return the major firmware version.""" + """Connect, authenticate, and return device info: fw_major, hostname, and mac."""
41-52: Log messages lump distinct failures.AirOSDataMissingError isn’t auth; split messages for clarity.
- except (AirOSConnectionAuthenticationError, AirOSDataMissingError): - _LOGGER.exception("Authentication error connecting to device at %s", host) + except AirOSConnectionAuthenticationError: + _LOGGER.exception("Authentication error connecting to device at %s", host) + raise + except AirOSDataMissingError: + _LOGGER.exception("Device returned incomplete data at %s", host) raiseairos/base.py (2)
58-59: LGTM! Consider documenting this public attribute.The
api_versionattribute provides useful version tracking, defaulting to 8 and updating to 6 after v6 XM login (line 372). Consider adding a docstring comment to clarify this is a public attribute that clients can inspect.
324-373: Three-step v6 XM login flow implemented correctly.The login sequence (session cookie → POST credentials → activate session) follows the expected v6 XM authentication pattern. The 302 redirect check (line 358) and session activation verification (line 369) provide appropriate error detection.
Consider making the URL check at line 369 more robust:
- if "login.cgi" in str(response.url): + if response.url.path and "login.cgi" in response.url.path:This ensures you're checking the path component specifically rather than the entire URL string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/verify.yml(1 hunks)CHANGELOG.md(1 hunks)airos/base.py(10 hunks)airos/data.py(7 hunks)airos/helpers.py(2 hunks)fixtures/airos_LiteBeam5AC_ap-ptp_30mhz.json(1 hunks)fixtures/airos_LiteBeam5AC_sta-ptp_30mhz.json(1 hunks)fixtures/airos_NanoBeam_5AC_ap-ptmp_v8.7.18.json(1 hunks)fixtures/airos_NanoStation_M5_sta_v6.3.16.json(5 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json(1 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json(1 hunks)fixtures/airos_liteapgps_ap_ptmp_40mhz.json(1 hunks)fixtures/airos_loco5ac_ap-ptp.json(1 hunks)fixtures/airos_loco5ac_sta-ptp.json(1 hunks)fixtures/airos_mocked_sta-ptmp.json(1 hunks)fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json(1 hunks)fixtures/airos_nanostation_ap-ptp_8718_missing_gps.json(1 hunks)pyproject.toml(1 hunks)tests/test_airos6.py(1 hunks)tests/test_airos8.py(2 hunks)tests/test_helpers.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/test_helpers.py
✅ Files skipped from review due to trivial changes (2)
- fixtures/airos_nanostation_ap-ptp_8718_missing_gps.json
- tests/test_airos6.py
🚧 Files skipped from review as they are similar to previous changes (2)
- fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (2)
airos/base.py (2)
airos/airos6.py (1)
_derived_wireless_data(39-54)airos/exceptions.py (5)
AirOSKeyDataMissingError(20-21)AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSUrlNotFoundError(44-45)AirOSConnectionSetupError(8-9)
airos/helpers.py (3)
airos/airos8.py (1)
AirOS8(11-30)airos/exceptions.py (5)
AirOSConnectionAuthenticationError(12-13)AirOSConnectionSetupError(8-9)AirOSDataMissingError(16-17)AirOSDeviceConnectionError(24-25)AirOSKeyDataMissingError(20-21)airos/base.py (4)
login(374-384)raw_status(386-388)get_fw_major(184-190)get_mac(193-220)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~10-~10: Ensure spelling is correct
Context: ...XW already was successful) - Calculated cpuload on v6 if values available (to prevent r...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (29)
.github/workflows/verify.yml (2)
177-177: Verify the rationale for lowering the coverage threshold.The required coverage threshold has been reduced from 85% to 80%. While the enriched summary indicates this aligns with PR activity (new tests/fixtures and removal of test_helpers.py), this represents a relaxation of code quality requirements.
Please clarify:
- Is this change intentional because new v6 XM code cannot meet the 85% threshold?
- Or is this a deliberate decision to accept a lower but still-adequate threshold?
- Has overall coverage metrics been assessed with the new changes?
9-14: Workflow trigger configuration note.The
pull_requesttrigger is commented out (line 14), meaning this workflow won't automatically run on pull requests—only on schedule, manual dispatch, or direct pushes. If this PR is intended to undergo CI validation, consider whether the trigger should be enabled for pull request events.fixtures/airos_NanoStation_M5_sta_v6.3.16.json (4)
147-147: All data model changes are properly supported—no breaking changes detected.The verification confirms the fixture is fully compatible:
frequency(int): Data model defines it asint; parsing code explicitly handles both string format"5180 MHz"and numeric format5180throughput(structured object): Data model defines it asThroughputtype; parsing code transformsrxrate/txratestrings into numericrx/txvalues- New fields (
fw_major,ieeemode, etc.): All defined in data models and present in the fixture- The fixture deserializes correctly with expected types
The original concerns about type consistency and missing model support have been verified as non-issues—the codebase already handles these changes.
25-27: CPU metrics are properly defined and handled—no issues found.The verification confirms that
cpubusy,cpuload, andcputotalare all properly defined in theHostdata model (airos/data.py, lines 249–251) with appropriate type annotations. These fields are also correctly handled during deserialization via the__pre_deserialize__hook (lines 260–263), which validates the fields and calculatescpuloadfromcpubusyandcputotalwhen both are present. The fixture data structure aligns with the model definition.
14-14: SKU mapping verified as correct; no action required.The fixture's devmodel
"NanoStation M5 "will correctly resolve to SKU"NSM5"through the mapper's fuzzy matching logic (suffix matching after lowercase comparison). The model_map.py contains the proper entry"airMAX NanoStation M5": "NSM5", and LocoM5 remains correctly assigned to loco variants only. No dangling references to the old SKU were found.
7-7: No issues found - fw_major field is properly defined and used.The
fw_majorfield is defined in theDerivedclass (airos/data.py:733) asfw_major: int | None = None. It is correctly set during response processing in airos/base.py:177 via theget_fw_major()method, and the fixture data structure matches the model definition.fixtures/airos_liteapgps_ap_ptmp_40mhz.json (1)
14-15: Consistent fw_major.Value 8 matches host.fwversion=v8.7.18.
fixtures/airos_loco5ac_sta-ptp.json (1)
14-15: LGTM.fw_major=8 aligns with v8.7.17.
fixtures/airos_NanoBeam_5AC_ap-ptmp_v8.7.18.json (1)
14-15: LGTM.fw_major=8 matches host v8.7.18; remote v6.x doesn’t affect host-derived value.
fixtures/airos_loco5ac_ap-ptp.json (1)
14-15: LGTM.fw_major=8 matches host major.
fixtures/airos_LiteBeam5AC_sta-ptp_30mhz.json (1)
14-15: LGTM.Value 8 matches v8.7.11.
airos/data.py (2)
145-151: Enum additions look fine.New IeeeMode members (_11ACVHT60, _11ACVHT50, _11NAHT40MINUS, _11NAHT40PLUS) align with observed device reports.
250-264: Nice: derive cpuload from cpubusy/cputotal.This prevents bogus near-100% cpuloads on XM.
fixtures/airos_mocked_sta-ptmp.json (1)
14-15: Verification complete—all fixtures' derived.fw_major correctly aligns with host.fwversion major version.Checked 13 fixtures containing both fields. All matches confirmed:
fixtures/airos_mocked_sta-ptmp.json: host.fwversion=v8.7.17 → derived.fw_major=8 ✓- All other fixtures similarly verified with correct alignment (v8.x.x→8, v6.x.x→6)
No mismatches found across the codebase.
fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json (1)
14-14: LGTM!The
fw_majorfield correctly reflects the major version fromfwversion: "v8.7.18"at line 46.fixtures/airos_LiteBeam5AC_ap-ptp_30mhz.json (1)
14-14: LGTM!The
fw_majorfield correctly reflects the major version fromfwversion: "v8.7.18"at line 37.tests/test_airos8.py (2)
126-131: LGTM!The addition of
fwversionis necessary to satisfy the newget_fw_majorhelper introduced inairos/base.py(lines 184-190), which is now called duringderived_dataprocessing.
260-260: LGTM!The addition of
fwversionensures the test fixture includes required fields for derived data processing.CHANGELOG.md (1)
5-16: LGTM!The changelog accurately documents the v6 XM login support, derived data enhancements, and firmware detection improvements introduced in this PR.
fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json (1)
1-154: LGTM!The v6 XM fixture is well-structured and demonstrates the key v6 features documented in the changelog:
- Correctly derived
fw_major: 6fromfwversion: "v6.3.16"antenna_gain: 13properly extracted fromantenna: "Built in - 13 dBi"- Both
ieeemodeandopmodefields present, enabling proper v6-to-v8 mappingthroughputobject withrx/txrates includedairos/base.py (9)
97-121: LGTM!Renaming to
_derived_wireless_datawith an underscore prefix correctly signals this is an internal helper method. The@staticmethoddecorator is appropriate as the method doesn't use instance state.
183-191: LGTM!The
get_fw_majorhelper correctly extracts the major version and handles edge cases appropriately with proper error handling.
192-220: LGTM!The
get_machelper correctly prioritizes interfaces (br0 → eth0 → ath0) and safely falls back to the first interface. The early return at line 197 ensuresinterfaces[0]access at lines 217-218 is safe.
222-225: LGTM!Converting
derived_datato a classmethod enables proper polymorphism, allowing subclasses (like AirOS6) to override_derived_wireless_datawhile sharing the common_derived_data_helperlogic.
242-245: LGTM!Cookie handling correctly preserves the full cookie name/value pair (e.g., "AIROS_XXX=value") as noted in past review feedback.
374-385: LGTM!The automatic fallback from v8 to v6 login (lines 379-380) provides seamless compatibility, allowing the same client code to work with both firmware versions without explicit version detection.
386-389: LGTM!The
raw_statusmethod provides a useful way to retrieve unprocessed device data, which can be helpful for debugging or when derived data processing isn't needed.
284-286: LGTM!Updating the connected check to use
_login_urlslist (line 284) correctly allows both v8 and v6 login endpoints to bypass the connection requirement.
165-178: LGTM!Refactoring to use static helpers (
get_macandget_fw_major) improves code modularity and testability while preserving the original logic.
|
@coderabbitai please update the summary and your walkthrough based on all information available, note that this will be 0.6.0 after testing (and no longer 0.6.0) so 0.5.7a14 will become 0.6.0. As well as a full review of the PR |
|
@CoMPaTech, I'll update the summary and walkthrough to reflect that this will be version 0.6.0, and provide a comprehensive review of the PR. Key ObservationsThis PR introduces significant enhancements to support v6 firmware XM models with a different login path, along with several architectural improvements: Version Note: The version should be updated from
Major Changes:
The scope of changes justifies a minor version bump to 0.6.0 given the breaking API changes. Note: This will be version 0.6.0 after testing. The version in pyproject.toml should be updated from 0.5.7a13 to 0.6.0, and the CHANGELOG.md entry for 0.5.7 should be changed to 0.6.0. ✅ Actions performedSummary regeneration triggered. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
12-12: Fix awkward phrasing and grammar on line 12.The phrase "XW already was successful" is grammatically awkward. Consider rephrasing to something like "XW was already supported" for improved clarity.
Apply this diff:
-Support for v6 firmware XM models using a different login path (XW already was successful) +Support for v6 firmware XM models using a different login path (XW was already supported)
14-14: Clarify the throughput availability statement on line 14.The phrasing states throughput is "available" but then "not yet available," which is contradictory. Clarify whether throughput is exposed in the current release or if it requires future work.
Consider rephrasing to something like:
-Added tx/rx rates for v6 as capacity (throughput is available in v6 web interface, but probably through counters, so not yet available) +Added tx/rx rates for v6 as capacity (throughput visible in v6 web UI but not yet exposed via API)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~12-~12: Ensure spelling is correct
Context: ...XW already was successful) - Calculated cpuload on v6 if values available (to prevent r...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (1)
CHANGELOG.md (1)
5-17: Changelog properly documents the 0.6.0 release.The changelog comprehensively covers the major features and improvements introduced in this PR: v6 XM login support, CPU load calculation, frequency handling, antenna derivation, and internal enhancements. The structure, acknowledgments, and organization align well with the PR objectives.
Summary by CodeRabbit
New Features
Data
Models
Tests
Chores