Add exception for legacy v6#206
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 24 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDetect legacy AirOS v6 TLS handshake failures in request handling and raise a new AirOSTLSCompatibilityError; helpers catch and re-raise this exception. Package version bumped to 0.6.8a0 and CHANGELOG updated for v0.6.8. ChangesTLS Compatibility Error Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 #206 +/- ##
==========================================
- Coverage 86.77% 85.27% -1.51%
==========================================
Files 8 8
Lines 756 774 +18
==========================================
+ Hits 656 660 +4
- Misses 100 114 +14 ☔ 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: 2
🧹 Nitpick comments (1)
airos/base.py (1)
524-536: 💤 Low value
err.__cause__inspection is redundant —ClientConnectorSSLErroralready IS anssl.SSLError.
ClientConnectorSSLErroris declared asclass ClientConnectorSSLError(*ssl_error_bases), wheressl_error_basesare(ClientSSLError, ssl.SSLError)— soerritself is anssl.SSLError. Additionally, aiohttp raises it asraise ClientConnectorSSLError(req.connection_key, exc) from exc, meaningerr.__cause__anderrcarry the same SSL error information.The
isinstance(err.__cause__, SSLError)branch is therefore always reachable whenerr.__cause__is set, but the same string content is already accessible directly onerr. You can simplify to a single-stage check usingstr(err).lower():♻️ Simplified helper
def _is_tls_compatibility_error(err: aiohttp.ClientConnectorSSLError) -> bool: """Return True for known legacy airOS TLS handshake failures.""" - if "SSLV3_ALERT_HANDSHAKE_FAILURE" in str(err): - return True - - cause = err.__cause__ - if isinstance(cause, SSLError): - message = str(cause).lower() - return ( - "handshake failure" in message or "sslv3 alert handshake failure" in message - ) - - return False + message = str(err).lower() + return "sslv3_alert_handshake_failure" in message or "handshake failure" in message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airos/base.py` around lines 524 - 536, The helper _is_tls_compatibility_error currently inspects err.__cause__, which is redundant because ClientConnectorSSLError already subclasses ssl.SSLError; update the logic to use the exception text from err directly (e.g., message = str(err).lower()) instead of checking err.__cause__, keep the existing top-level check for "SSLV3_ALERT_HANDSHAKE_FAILURE", and then return True if "handshake failure" or "sslv3 alert handshake failure" appears in message so the function relies on err itself rather than its __cause__.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@airos/exceptions.py`:
- Around line 52-53: The docstring for class AirOSTLSCompatibilityError (which
subclasses AirOSDeviceConnectionError) contains a typo: "compaitibility" should
be "compatibility"; update the class docstring to read "The device requires
legacy TLS/cipher compatibility." ensuring the corrected spelling is used in
AirOSTLSCompatibilityError.
In `@pyproject.toml`:
- Line 7: Update the version label so the pyproject.toml value and the CHANGELOG
entry match: either change the pyproject.toml version string "version =
\"0.6.6a0\"" to "0.6.6" or edit the CHANGELOG heading "[0.6.6]" to "[0.6.6a0]";
ensure both places use the exact same version token before merging the draft to
avoid PEP 440 sorting issues on PyPI.
---
Nitpick comments:
In `@airos/base.py`:
- Around line 524-536: The helper _is_tls_compatibility_error currently inspects
err.__cause__, which is redundant because ClientConnectorSSLError already
subclasses ssl.SSLError; update the logic to use the exception text from err
directly (e.g., message = str(err).lower()) instead of checking err.__cause__,
keep the existing top-level check for "SSLV3_ALERT_HANDSHAKE_FAILURE", and then
return True if "handshake failure" or "sslv3 alert handshake failure" appears in
message so the function relies on err itself rather than its __cause__.
🪄 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: CHILL
Plan: Pro
Run ID: 96ea020f-dfd4-4676-941b-887445d30ab9
📒 Files selected for processing (4)
CHANGELOG.mdairos/base.pyairos/exceptions.pypyproject.toml
| [project] | ||
| name = "airos" | ||
| version = "0.6.5" | ||
| version = "0.6.6a0" |
There was a problem hiding this comment.
Version label in pyproject.toml (0.6.6a0) doesn't match the CHANGELOG entry ([0.6.6]).
0.6.6a0 is a PEP 440 alpha pre-release. PyPI will sort it below 0.6.6. Since this is a draft, the a0 suffix is likely a work-in-progress marker, but the CHANGELOG entry (Line 5) should align — either record [0.6.6a0] there, or drop the suffix in pyproject.toml before the PR graduates from draft.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` at line 7, Update the version label so the pyproject.toml
value and the CHANGELOG entry match: either change the pyproject.toml version
string "version = \"0.6.6a0\"" to "0.6.6" or edit the CHANGELOG heading
"[0.6.6]" to "[0.6.6a0]"; ensure both places use the exact same version token
before merging the draft to avoid PEP 440 sorting issues on PyPI.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
airos/helpers.py (1)
42-44: 💤 Low valueRedundant logging of TLS compatibility error.
The exception is already logged in
base.py(_request_json) before being raised. This handler logs the same error again, resulting in duplicate log entries for a single failure.Consider removing the logging here since the exception already carries the context, or keeping only one log site for clarity.
♻️ Proposed fix to remove redundant logging
except AirOSTLSCompatibilityError: - _LOGGER.exception("TLS compatibility error during API call to %s", host) raise🤖 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 `@airos/helpers.py` around lines 42 - 44, The except block for AirOSTLSCompatibilityError in airos/helpers.py is redundantly logging the same error already logged by _request_json in base.py; remove the _LOGGER.exception("TLS compatibility error during API call to %s", host) line and leave only the raise so the exception bubbles up (preserving original context from _request_json), referencing the AirOSTLSCompatibilityError handler in airos/helpers.py and the _request_json logging in base.py to locate the duplicate log site.
🤖 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.
Nitpick comments:
In `@airos/helpers.py`:
- Around line 42-44: The except block for AirOSTLSCompatibilityError in
airos/helpers.py is redundantly logging the same error already logged by
_request_json in base.py; remove the _LOGGER.exception("TLS compatibility error
during API call to %s", host) line and leave only the raise so the exception
bubbles up (preserving original context from _request_json), referencing the
AirOSTLSCompatibilityError handler in airos/helpers.py and the _request_json
logging in base.py to locate the duplicate log site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c957ef3d-51a2-4701-a08b-618983523bab
📒 Files selected for processing (5)
CHANGELOG.mdairos/base.pyairos/exceptions.pyairos/helpers.pypyproject.toml
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
|
1 participant
In addition to #205
Summary by CodeRabbit
New Features
Chores