feat: Add INTL region support and EU token refresh#177
Open
onpremcloudguy wants to merge 2 commits into
Open
Conversation
Add support for Hello Smart International (INTL) API used in Australia, Singapore, Israel, and other international markets. - Add SmartAuthenticationINTL class with 3-step auth flow - Implement EU token refresh via /login-app/api/v1/token - Add clear_credentials() security method - Add V2L, basic_status, trailer vehicle data - Add SmartRegion enum for region selection - Update .gitignore for dev/docker folders
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds INTL-region support (authentication, signing, endpoints), region-aware header generation and credential clearing; introduces BasicStatus and Trailer vehicle models; extends Battery and Climate telemetry fields; updates SmartVehicle to include new models and adjusts parsing for regional differences. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SmartAccount
participant AuthINTL as SmartAuthenticationINTL
participant INTL_Auth_Service as INTL Auth Service
participant INTL_OAuth as INTL OAuth
participant VehicleAPI as Vehicle API
Client->>SmartAccount: initialize(region=INTL)
SmartAccount->>AuthINTL: instantiate (region-aware)
Client->>AuthINTL: login(username, password)
AuthINTL->>INTL_Auth_Service: POST /login (credentials)
INTL_Auth_Service-->>AuthINTL: intl_access_token, intl_id_token
AuthINTL->>INTL_OAuth: POST /oauth/token (exchange)
INTL_OAuth-->>AuthINTL: authorization/session token
AuthINTL->>VehicleAPI: POST /session (exchange for vehicle API tokens)
VehicleAPI-->>AuthINTL: api_access_token, api_refresh_token
AuthINTL-->>SmartAccount: return tokens
SmartAccount-->>Client: authenticated
sequenceDiagram
participant Client
participant SmartAccount
participant Utils as API Utils
participant VehicleAPI as Vehicle API
participant Parser as Vehicle Parsers
Client->>SmartAccount: get_vehicle_data()
SmartAccount->>Utils: generate_intl_header(device_id, access_token, params)
Utils->>Utils: compute nonce & timestamp & signature (INTL_SECRET)
Utils-->>SmartAccount: headers
SmartAccount->>VehicleAPI: GET /vehicle (with INTL headers)
VehicleAPI-->>SmartAccount: vehicle payload
SmartAccount->>Parser: BasicStatus.from_vehicle_data / Trailer.from_vehicle_data / Battery/Climate parsing
Parser-->>SmartAccount: parsed models
SmartAccount-->>Client: return parsed models
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
pysmarthashtag/vehicle/climate.py (1)
215-217: Reduce log noise for optional pollution fields.On Line 215–Line 217,
get_field_as_typeuses its defaultlog_missing=True. These keys are optional by market, so this can generate repetitive missing-field logs during normal polling. Consider settinglog_missing=Falsefor these three lookups.♻️ Suggested change
- retval["interior_PM25_level"] = get_field_as_type(pollutionStatus, "interiorPM25Level", int) - retval["interior_secondary_PM25_level"] = get_field_as_type(pollutionStatus, "interiorSecondPM25Level", int) - retval["exterior_PM25_level"] = get_field_as_type(pollutionStatus, "exteriorPM25Level", int) + retval["interior_PM25_level"] = get_field_as_type( + pollutionStatus, "interiorPM25Level", int, log_missing=False + ) + retval["interior_secondary_PM25_level"] = get_field_as_type( + pollutionStatus, "interiorSecondPM25Level", int, log_missing=False + ) + retval["exterior_PM25_level"] = get_field_as_type( + pollutionStatus, "exteriorPM25Level", int, log_missing=False + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysmarthashtag/vehicle/climate.py` around lines 215 - 217, The three pollution-related optional fields are currently fetched with get_field_as_type using its default log_missing=True which floods logs; update the three assignments that set retval["interior_PM25_level"], retval["interior_secondary_PM25_level"], and retval["exterior_PM25_level"] to call get_field_as_type(pollutionStatus, "<fieldName>", int, log_missing=False) (using the exact keys "interiorPM25Level", "interiorSecondPM25Level", "exteriorPM25Level") so missing optional fields are not logged during normal polling.pysmarthashtag/const.py (1)
79-84: Use existing INTL constants in the INTL endpoint preset.Line 79–Line 84 duplicate URL literals already declared as constants, which increases drift risk.
♻️ Proposed refactor
return EndpointUrls( - server_url="https://sg-app-api.smart.com/iam/service/api/v1/login", - auth_url="https://sg-app-api.smart.com/iam/service/api/v1/oauth20/authorize", - login_url="https://sg-app-api.smart.com/iam/service/api/v1/login", + server_url=INTL_LOGIN_URL, + auth_url=INTL_OAUTH_URL, + login_url=INTL_LOGIN_URL, # INTL uses same data endpoints as EU - api_base_url="https://api.ecloudeu.com", - api_base_url_v2="https://apiv2.ecloudeu.com", + api_base_url=API_BASE_URL, + api_base_url_v2=API_BASE_URL_V2, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysmarthashtag/const.py` around lines 79 - 84, The INTL endpoint preset currently reuses hard-coded URL literals for server_url, auth_url, login_url, api_base_url and api_base_url_v2; replace these duplicated string literals with the existing INTL constants already defined elsewhere in the module (use the corresponding INTL_* constant names instead of the literal URLs) so the preset references the canonical constants and avoids drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pysmarthashtag/account.py`:
- Around line 87-90: The INTL signed header is built with the wrong device id:
replace use of auth.device_id with the INTL auth object's device_identifier when
calling utils.generate_intl_header (i.e., ensure the code path that uses
SmartAuthenticationINTL passes auth.device_identifier into device_id argument),
so generate_intl_header, SmartAuthenticationINTL, and the auth.device_identifier
field are used consistently for INTL signed vehicle API requests.
In `@pysmarthashtag/api/authentication.py`:
- Around line 750-755: The returned payload is assigning the IAM refresh token
key incorrectly — replace the value for "refresh_token" with the INTL refresh
token (self.intl_refresh_token) while keeping "api_refresh_token" as
api_refresh_token; update the same incorrect mapping in the other spot mentioned
(around the other return block). Locate the return dicts in authentication.py
(the function/method that builds the token response, and the similar block near
the second occurrence) and change the "refresh_token" value to
self.intl_refresh_token so INTL refresh flow uses the correct token while
leaving "api_refresh_token" and "api_access_token" unchanged.
- Around line 676-709: The code currently logs raw INTL auth artifacts (e.g.,
self.intl_user_id, self.intl_access_token, self.intl_id_token, auth_code and
full oauth_result) inside the INTL login flow; update the logging in the method
handling INTL login (the block that calls _LOGGER.info("INTL: Login
successful..."), _LOGGER.debug("INTL OAuth response code...") and
_LOGGER.debug("INTL: Got authCode...")) to stop emitting raw secrets and
identifiers: import and call sanitize_log_data() from api.log_sanitizer for any
user/id/token values you must log, or remove the value from the log message
entirely, and ensure errors log only sanitized oauth_result.get("message")
(sanitized) and not full token payloads.
In `@pysmarthashtag/api/utils.py`:
- Around line 11-15: EU_SECRET and INTL_SECRET are hard-coded secrets; remove
these literal values from pysmarthashtag.api.utils and load them from secure
configuration (e.g., environment variables or a secrets manager) instead. Change
the code that defines EU_SECRET and INTL_SECRET to read from environment
variables like SMARTHASHTAG_EU_SECRET and SMARTHASHTAG_INTL_SECRET
(base64-decode SMARTHASHTAG_EU_SECRET when required, and .encode("utf-8") or
accept bytes for INTL_SECRET), validate they are present and raise a clear error
if missing, and update any callers of EU_SECRET/INTL_SECRET to use the new
variables; also update deployment/docs and tests to inject mock secrets via env
vars or fixtures.
In `@pysmarthashtag/vehicle/basic_status.py`:
- Around line 149-152: The current try/except/finally suppresses non-KeyError
exceptions because the return is inside finally; move the return retval out of
the finally block in the function that constructs retval (the block with except
KeyError as e and _LOGGER.info) so that only KeyError is caught and logged via
_LOGGER.info while other exceptions (e.g., ValueError from
datetime.fromtimestamp calls) are allowed to propagate (or are explicitly
handled/re-raised). Ensure the except only handles KeyError, remove the finally
or keep it for cleanup only, and place the final "return retval" after the
try/except so errors aren't swallowed.
In `@pysmarthashtag/vehicle/trailer.py`:
- Around line 68-71: The finally block currently contains "return retval" which
suppresses exceptions and overrides the intended early return/exception
behavior; remove the return from the finally block and instead return retval
after the try/except/finally completes (or return/raise inside the try/except
branches as appropriate). Concretely, edit the function that uses the local
variable retval and the except KeyError as e handler so the except either sets
retval (e.g., to None) or re-raises the exception, then place a single "return
retval" statement after the try/except/finally so that exceptions are not
silenced and the early return behavior is preserved. Ensure no other code relies
on a return inside finally.
---
Nitpick comments:
In `@pysmarthashtag/const.py`:
- Around line 79-84: The INTL endpoint preset currently reuses hard-coded URL
literals for server_url, auth_url, login_url, api_base_url and api_base_url_v2;
replace these duplicated string literals with the existing INTL constants
already defined elsewhere in the module (use the corresponding INTL_* constant
names instead of the literal URLs) so the preset references the canonical
constants and avoids drift.
In `@pysmarthashtag/vehicle/climate.py`:
- Around line 215-217: The three pollution-related optional fields are currently
fetched with get_field_as_type using its default log_missing=True which floods
logs; update the three assignments that set retval["interior_PM25_level"],
retval["interior_secondary_PM25_level"], and retval["exterior_PM25_level"] to
call get_field_as_type(pollutionStatus, "<fieldName>", int, log_missing=False)
(using the exact keys "interiorPM25Level", "interiorSecondPM25Level",
"exteriorPM25Level") so missing optional fields are not logged during normal
polling.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignorepysmarthashtag/account.pypysmarthashtag/api/authentication.pypysmarthashtag/api/log_sanitizer.pypysmarthashtag/api/utils.pypysmarthashtag/const.pypysmarthashtag/vehicle/basic_status.pypysmarthashtag/vehicle/battery.pypysmarthashtag/vehicle/climate.pypysmarthashtag/vehicle/safety.pypysmarthashtag/vehicle/trailer.pypysmarthashtag/vehicle/vehicle.py
Comment on lines
+87
to
+90
| return utils.generate_intl_header( | ||
| device_id=auth.device_id, | ||
| access_token=auth.api_access_token, | ||
| params=params, |
Contributor
There was a problem hiding this comment.
Use the INTL device identifier for INTL signed headers.
Line 88 currently uses auth.device_id, but SmartAuthenticationINTL establishes INTL auth/session with device_identifier. Keep this identifier consistent for signed vehicle API requests.
🔧 Proposed fix
return utils.generate_intl_header(
- device_id=auth.device_id,
+ device_id=getattr(auth, "device_identifier", auth.device_id),
access_token=auth.api_access_token,
params=params,
method=method,
url=url,
body=body,
client_id=client_id,
)📝 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.
Suggested change
| return utils.generate_intl_header( | |
| device_id=auth.device_id, | |
| access_token=auth.api_access_token, | |
| params=params, | |
| return utils.generate_intl_header( | |
| device_id=getattr(auth, "device_identifier", auth.device_id), | |
| access_token=auth.api_access_token, | |
| params=params, | |
| method=method, | |
| url=url, | |
| body=body, | |
| client_id=client_id, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysmarthashtag/account.py` around lines 87 - 90, The INTL signed header is
built with the wrong device id: replace use of auth.device_id with the INTL auth
object's device_identifier when calling utils.generate_intl_header (i.e., ensure
the code path that uses SmartAuthenticationINTL passes auth.device_identifier
into device_id argument), so generate_intl_header, SmartAuthenticationINTL, and
the auth.device_identifier field are used consistently for INTL signed vehicle
API requests.
Comment on lines
+676
to
+709
| _LOGGER.info("INTL: Login successful for user %s", self.intl_user_id) | ||
|
|
||
| except (KeyError, ValueError, TypeError) as e: | ||
| raise SmartAPIError(f"Could not parse INTL login response: {e}") | ||
|
|
||
| # Step 2: Exchange accessToken for authCode via OAuth | ||
| # Note: OAuth uses just accessToken in URL param, and idToken in Xs-Auth-Token header | ||
| _LOGGER.debug("INTL: Step 2 - Exchanging accessToken for authCode") | ||
|
|
||
| oauth_url = f"{INTL_OAUTH_URL}?accessToken={self.intl_access_token}" | ||
|
|
||
| oauth_headers = self._generate_intl_headers(self.intl_id_token) | ||
|
|
||
| r_oauth = await client.get( | ||
| oauth_url, | ||
| headers=oauth_headers, | ||
| ) | ||
|
|
||
| try: | ||
| oauth_result = r_oauth.json() | ||
| _LOGGER.debug("INTL OAuth response code: %s", oauth_result.get("code")) | ||
|
|
||
| if oauth_result.get("code") != "200": | ||
| error_msg = oauth_result.get("message", "Unknown error") | ||
| _LOGGER.error("INTL OAuth failed: %s", error_msg) | ||
| raise SmartAPIError(f"INTL OAuth failed: {error_msg}") | ||
|
|
||
| # Result is directly the authCode string (e.g., "CODE-xxx") | ||
| auth_code = oauth_result.get("result") | ||
| if isinstance(auth_code, dict): | ||
| auth_code = auth_code.get("authCode", auth_code) | ||
|
|
||
| _LOGGER.debug("INTL: Got authCode: %s...", str(auth_code)[:30] if auth_code else "None") | ||
|
|
Contributor
There was a problem hiding this comment.
Stop logging raw INTL auth/user artifacts.
Line 676, Line 708, Line 745, and Line 804 log sensitive identity/auth values directly. Remove these values or sanitize before logging.
🔒 Proposed logging hardening
- _LOGGER.info("INTL: Login successful for user %s", self.intl_user_id)
+ _LOGGER.info("INTL: Login successful")
- _LOGGER.debug("INTL: Got authCode: %s...", str(auth_code)[:30] if auth_code else "None")
+ _LOGGER.debug("INTL: OAuth authCode received")
- _LOGGER.info("INTL: Successfully authenticated, user ID: %s", api_user_id)
+ _LOGGER.info("INTL: Successfully authenticated")
- _LOGGER.info("INTL: Token refresh successful for user %s", self.intl_user_id)
+ _LOGGER.info("INTL: Token refresh successful")Also applies to: 745-746, 804-805
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 679-679: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 679-679: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 701-701: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysmarthashtag/api/authentication.py` around lines 676 - 709, The code
currently logs raw INTL auth artifacts (e.g., self.intl_user_id,
self.intl_access_token, self.intl_id_token, auth_code and full oauth_result)
inside the INTL login flow; update the logging in the method handling INTL login
(the block that calls _LOGGER.info("INTL: Login successful..."),
_LOGGER.debug("INTL OAuth response code...") and _LOGGER.debug("INTL: Got
authCode...")) to stop emitting raw secrets and identifiers: import and call
sanitize_log_data() from api.log_sanitizer for any user/id/token values you must
log, or remove the value from the log message entirely, and ensure errors log
only sanitized oauth_result.get("message") (sanitized) and not full token
payloads.
Comment on lines
+750
to
+755
| return { | ||
| "access_token": api_access_token, | ||
| "refresh_token": api_refresh_token, | ||
| "api_access_token": api_access_token, | ||
| "api_refresh_token": api_refresh_token, | ||
| "api_user_id": api_user_id, |
Contributor
There was a problem hiding this comment.
INTL refresh_token payload currently returns the wrong token type.
Line 752 and Line 862 return api_refresh_token as refresh_token, but INTL refresh flow depends on IAM refresh token (self.intl_refresh_token). This breaks refresh continuity after object re-instantiation.
🔁 Proposed token mapping fix
return {
"access_token": api_access_token,
- "refresh_token": api_refresh_token,
+ "refresh_token": self.intl_refresh_token,
"api_access_token": api_access_token,
"api_refresh_token": api_refresh_token,
"api_user_id": api_user_id,
"expires_at": expires_at,
}
@@
return {
"access_token": api_access_token,
- "refresh_token": api_refresh_token,
+ "refresh_token": self.intl_refresh_token,
"api_access_token": api_access_token,
"api_refresh_token": api_refresh_token,
"api_user_id": api_user_id,
"expires_at": expires_at,
}Also applies to: 860-865
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysmarthashtag/api/authentication.py` around lines 750 - 755, The returned
payload is assigning the IAM refresh token key incorrectly — replace the value
for "refresh_token" with the INTL refresh token (self.intl_refresh_token) while
keeping "api_refresh_token" as api_refresh_token; update the same incorrect
mapping in the other spot mentioned (around the other return block). Locate the
return dicts in authentication.py (the function/method that builds the token
response, and the similar block near the second occurrence) and change the
"refresh_token" value to self.intl_refresh_token so INTL refresh flow uses the
correct token while leaving "api_refresh_token" and "api_access_token"
unchanged.
Comment on lines
+11
to
+15
| # EU signing secret (base64 encoded) | ||
| EU_SECRET = base64.b64decode("NzRlNzQ2OWFmZjUwNDJiYmJlZDdiYmIxYjM2YzE1ZTk=") | ||
|
|
||
| # INTL (International) signing secret - used for Australia, Singapore, Israel, etc. | ||
| INTL_SECRET = "30fb4a7b7fab4e2e8b52120d0087efdd".encode("utf-8") |
Contributor
There was a problem hiding this comment.
Move signing secrets out of source code.
Line 12 and Line 15 commit signing secrets directly in the repository. This is a security risk and already surfaced by secret scanning.
🔐 Proposed direction
+import os
import base64
import hashlib
import hmac
@@
-# EU signing secret (base64 encoded)
-EU_SECRET = base64.b64decode("NzRlNzQ2OWFmZjUwNDJiYmJlZDdiYmIxYjM2YzE1ZTk=")
+# EU signing secret (base64 encoded) - load from environment/secret manager
+EU_SECRET = base64.b64decode(os.environ["SMART_EU_SIGNING_SECRET_B64"])
@@
-# INTL (International) signing secret - used for Australia, Singapore, Israel, etc.
-INTL_SECRET = "30fb4a7b7fab4e2e8b52120d0087efdd".encode("utf-8")
+# INTL signing secret - load from environment/secret manager
+INTL_SECRET = os.environ["SMART_INTL_SIGNING_SECRET"].encode("utf-8")🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysmarthashtag/api/utils.py` around lines 11 - 15, EU_SECRET and INTL_SECRET
are hard-coded secrets; remove these literal values from
pysmarthashtag.api.utils and load them from secure configuration (e.g.,
environment variables or a secrets manager) instead. Change the code that
defines EU_SECRET and INTL_SECRET to read from environment variables like
SMARTHASHTAG_EU_SECRET and SMARTHASHTAG_INTL_SECRET (base64-decode
SMARTHASHTAG_EU_SECRET when required, and .encode("utf-8") or accept bytes for
INTL_SECRET), validate they are present and raise a clear error if missing, and
update any callers of EU_SECRET/INTL_SECRET to use the new variables; also
update deployment/docs and tests to inject mock secrets via env vars or
fixtures.
Comment on lines
+149
to
+152
| except KeyError as e: | ||
| _LOGGER.info(f"Basic status info not available: {e}") | ||
| finally: | ||
| return retval |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd basic_status.py --type fRepository: DasBasti/pySmartHashtag
Length of output: 105
🏁 Script executed:
cat -n pysmarthashtag/vehicle/basic_status.py | sed -n '130,165p'Repository: DasBasti/pySmartHashtag
Length of output: 1249
🏁 Script executed:
cat -n pysmarthashtag/vehicle/basic_status.py | sed -n '80,155p'Repository: DasBasti/pySmartHashtag
Length of output: 4226
🏁 Script executed:
cat -n pysmarthashtag/vehicle/basic_status.py | head -10Repository: DasBasti/pySmartHashtag
Length of output: 427
🏁 Script executed:
rg "get_field_as_type" --type py -A 3 -B 1 | head -40Repository: DasBasti/pySmartHashtag
Length of output: 2613
🏁 Script executed:
cat -n pysmarthashtag/models.py | grep -A 20 "def get_field_as_type"Repository: DasBasti/pySmartHashtag
Length of output: 972
🏁 Script executed:
rg "except.*:\s*\n\s*_LOGGER" pysmarthashtag/vehicle/ --type py -B 5 -A 5Repository: DasBasti/pySmartHashtag
Length of output: 258
🏁 Script executed:
rg "except KeyError" pysmarthashtag/vehicle/ --type py -A 10Repository: DasBasti/pySmartHashtag
Length of output: 2723
🏁 Script executed:
cat -n pysmarthashtag/vehicle/basic_status.py | sed -n '118,125p'Repository: DasBasti/pySmartHashtag
Length of output: 525
Move return outside finally block to prevent exception suppression.
The finally block at line 151–152 will suppress any exception raised during parsing that isn't a KeyError—for example, ValueError from datetime.fromtimestamp() at lines 120, 137, and 147. This hides errors and makes troubleshooting difficult.
Fix
- except KeyError as e:
- _LOGGER.info(f"Basic status info not available: {e}")
- finally:
- return retval
+ except KeyError as e:
+ _LOGGER.info(f"Basic status info not available: {e}")
+ return retval📝 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.
Suggested change
| except KeyError as e: | |
| _LOGGER.info(f"Basic status info not available: {e}") | |
| finally: | |
| return retval | |
| except KeyError as e: | |
| _LOGGER.info(f"Basic status info not available: {e}") | |
| return retval |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 152-152: return inside finally blocks cause exceptions to be silenced
(B012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysmarthashtag/vehicle/basic_status.py` around lines 149 - 152, The current
try/except/finally suppresses non-KeyError exceptions because the return is
inside finally; move the return retval out of the finally block in the function
that constructs retval (the block with except KeyError as e and _LOGGER.info) so
that only KeyError is caught and logged via _LOGGER.info while other exceptions
(e.g., ValueError from datetime.fromtimestamp calls) are allowed to propagate
(or are explicitly handled/re-raised). Ensure the except only handles KeyError,
remove the finally or keep it for cleanup only, and place the final "return
retval" after the try/except so errors aren't swallowed.
Comment on lines
+68
to
+71
| except KeyError as e: | ||
| _LOGGER.info(f"Trailer status info not available: {e}") | ||
| finally: | ||
| return retval |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and check the specific lines mentioned
echo "=== File check ==="
if [ -f "pysmarthashtag/vehicle/trailer.py" ]; then
echo "File found"
wc -l pysmarthashtag/vehicle/trailer.py
else
echo "File not found"
exit 1
fi
echo -e "\n=== Lines 50-75 (context around lines 68-71) ==="
sed -n '50,75p' pysmarthashtag/vehicle/trailer.py | cat -n
echo -e "\n=== Check for return statements in finally blocks ==="
rg -nU 'finally:\n\s*return' pysmarthashtag/vehicle/trailer.pyRepository: DasBasti/pySmartHashtag
Length of output: 1579
Avoid return in finally here—it suppresses exceptions and overrides the early return at line 54.
A return statement in the finally block (lines 70–71) overrides the early return None at line 54 and suppresses the caught KeyError exception, silencing potential parsing failures.
Suggested fix
except KeyError as e:
_LOGGER.info(f"Trailer status info not available: {e}")
- finally:
- return retval
+ return retval🧰 Tools
🪛 Ruff (0.15.2)
[warning] 71-71: return inside finally blocks cause exceptions to be silenced
(B012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysmarthashtag/vehicle/trailer.py` around lines 68 - 71, The finally block
currently contains "return retval" which suppresses exceptions and overrides the
intended early return/exception behavior; remove the return from the finally
block and instead return retval after the try/except/finally completes (or
return/raise inside the try/except branches as appropriate). Concretely, edit
the function that uses the local variable retval and the except KeyError as e
handler so the except either sets retval (e.g., to None) or re-raises the
exception, then place a single "return retval" statement after the
try/except/finally so that exceptions are not silenced and the early return
behavior is preserved. Ensure no other code relies on a return inside finally.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pysmarthashtag/account.py (1)
87-95:⚠️ Potential issue | 🟠 MajorUse
device_identifierfor INTL signed headers.Line 88 uses
auth.device_id, butSmartAuthenticationINTLestablishes the INTL auth/session withdevice_identifier. For consistency with INTL signing, use the INTL-specific identifier.🔧 Proposed fix
if self._is_intl_region(): # INTL region uses different headers and signing client_id = getattr(auth, 'api_client_id', None) return utils.generate_intl_header( - device_id=auth.device_id, + device_id=getattr(auth, "device_identifier", auth.device_id), access_token=auth.api_access_token, params=params, method=method, url=url, body=body, client_id=client_id, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysmarthashtag/account.py` around lines 87 - 95, The INTL-signed header call is using auth.device_id but SmartAuthenticationINTL establishes the session with a different identifier; update the call to utils.generate_intl_header to pass the INTL-specific identifier (auth.device_identifier) instead of auth.device_id so signing uses the correct device token; locate the call in account.py where generate_intl_header is invoked and replace the device_id argument to reference auth.device_identifier (ensure any tests or callers expecting device_id are adjusted to the INTL field if needed).pysmarthashtag/api/authentication.py (2)
676-676:⚠️ Potential issue | 🟠 MajorAvoid logging sensitive identifiers directly.
Lines 676, 708, 745, and 804 log user IDs and partial auth codes directly. These should be removed or sanitized per coding guidelines.
🔒 Proposed logging hardening
- _LOGGER.info("INTL: Login successful for user %s", self.intl_user_id) + _LOGGER.info("INTL: Login successful")- _LOGGER.debug("INTL: Got authCode: %s...", str(auth_code)[:30] if auth_code else "None") + _LOGGER.debug("INTL: OAuth authCode received")- _LOGGER.info("INTL: Successfully authenticated, user ID: %s", api_user_id) + _LOGGER.info("INTL: Successfully authenticated")- _LOGGER.info("INTL: Token refresh successful for user %s", self.intl_user_id) + _LOGGER.info("INTL: Token refresh successful")Based on learnings: Use
sanitize_log_data()fromapi.log_sanitizerwhen logging sensitive data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysmarthashtag/api/authentication.py` at line 676, Replace direct logging of sensitive identifiers with sanitized values using sanitize_log_data from api.log_sanitizer: update calls that use _LOGGER.info/_LOGGER.debug which include self.intl_user_id and any auth code variables (e.g., partial auth code variables logged in the same class/methods where the "INTL: Login successful for user %s" message appears) to pass sanitize_log_data(self.intl_user_id) or sanitize_log_data(auth_code_var) instead, or remove the log entry entirely if not needed; import sanitize_log_data at the top of the module and ensure every occurrence that logs user IDs or auth codes uses that sanitizer.
750-757:⚠️ Potential issue | 🟠 MajorConsider returning
intl_refresh_tokenfor refresh continuity.The return dict sets
refresh_tokentoapi_refresh_token, but INTL refresh flow depends onself.intl_refresh_token. If the auth object is re-instantiated with stored tokens, the INTL refresh won't work becauseintl_refresh_tokenwon't be available.🔁 Proposed token mapping fix
return { "access_token": api_access_token, - "refresh_token": api_refresh_token, + "refresh_token": self.intl_refresh_token, "api_access_token": api_access_token, "api_refresh_token": api_refresh_token, "api_user_id": api_user_id, "expires_at": expires_at, }Also apply the same fix at line 862.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysmarthashtag/api/authentication.py` around lines 750 - 757, The returned token dict currently maps "refresh_token" to api_refresh_token which breaks INTL refresh flow; change the mapping so "refresh_token" uses self.intl_refresh_token (and include "intl_refresh_token": self.intl_refresh_token) while keeping "api_refresh_token": api_refresh_token and "api_access_token": api_access_token; update the return that builds {"access_token": api_access_token, "refresh_token": ..., "api_access_token": ..., "api_refresh_token": ..., "api_user_id": api_user_id, "expires_at": expires_at} to use self.intl_refresh_token for the refresh_token and add the intl_refresh_token key, and make the same corresponding change for the duplicate token-returning block later in the file that uses the same variables (api_access_token, api_refresh_token, api_user_id, expires_at).
🧹 Nitpick comments (2)
pysmarthashtag/api/authentication.py (2)
678-679: Chain exceptions withraise ... fromfor better tracebacks.When re-raising exceptions, use
raise ... from errto preserve the exception chain and improve debugging.♻️ Proposed fix
except (KeyError, ValueError, TypeError) as e: - raise SmartAPIError(f"Could not parse INTL login response: {e}") + raise SmartAPIError(f"Could not parse INTL login response: {e}") from eApply the same pattern at lines 711 and 748.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysmarthashtag/api/authentication.py` around lines 678 - 679, Change the bare re-raises of parsing exceptions to chain the original exception using "raise ... from e": in the except blocks (e.g., the block that catches (KeyError, ValueError, TypeError) and raises SmartAPIError("Could not parse INTL login response: {e}")), update the raise to use "raise SmartAPIError(...) from e". Apply the same pattern for the other similar except blocks referenced in the file (the ones at the locations corresponding to lines around 711 and 748) so all SmartAPIError raises preserve the original exception chain.
549-550: Consider moving imports to module level.The
timeanduuidimports are placed inside the method. While functional, module-level imports are more conventional and slightly more efficient.♻️ Suggested improvement
Move the imports to the top of the file with other imports:
import asyncio import datetime import json import logging import math import secrets import ssl +import time +import uuid from collections import defaultdictThen remove the local imports in
_generate_intl_headersand_generate_vehicle_api_headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysmarthashtag/api/authentication.py` around lines 549 - 550, Move the local imports of time and uuid out of the methods and into the module-level imports at the top of the file (add "import time" and "import uuid" alongside the other imports), then remove the in-function "import time" and "import uuid" lines from _generate_intl_headers and _generate_vehicle_api_headers so both functions use the module-level symbols; verify there are no other local import duplicates and run tests to ensure no naming conflicts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pysmarthashtag/api/authentication.py`:
- Line 579: The parameter annotation in _generate_vehicle_api_headers currently
uses an implicit Optional via body: str = None; change the signature to
explicitly declare Optional[str] (e.g., body: Optional[str] = None) and add the
corresponding typing import (from typing import Optional) or use the explicit
union syntax (str | None) if the project targets Python 3.10+; update any
references or type checks in the function if needed to match the new annotation.
In `@pysmarthashtag/const.py`:
- Around line 35-46: The SmartRegion enum currently inherits from Enum but Enum
is not imported; add the missing import for Enum (from the standard library enum
module) at the top of the file so SmartRegion (class SmartRegion) can reference
Enum without causing a NameError.
---
Duplicate comments:
In `@pysmarthashtag/account.py`:
- Around line 87-95: The INTL-signed header call is using auth.device_id but
SmartAuthenticationINTL establishes the session with a different identifier;
update the call to utils.generate_intl_header to pass the INTL-specific
identifier (auth.device_identifier) instead of auth.device_id so signing uses
the correct device token; locate the call in account.py where
generate_intl_header is invoked and replace the device_id argument to reference
auth.device_identifier (ensure any tests or callers expecting device_id are
adjusted to the INTL field if needed).
In `@pysmarthashtag/api/authentication.py`:
- Line 676: Replace direct logging of sensitive identifiers with sanitized
values using sanitize_log_data from api.log_sanitizer: update calls that use
_LOGGER.info/_LOGGER.debug which include self.intl_user_id and any auth code
variables (e.g., partial auth code variables logged in the same class/methods
where the "INTL: Login successful for user %s" message appears) to pass
sanitize_log_data(self.intl_user_id) or sanitize_log_data(auth_code_var)
instead, or remove the log entry entirely if not needed; import
sanitize_log_data at the top of the module and ensure every occurrence that logs
user IDs or auth codes uses that sanitizer.
- Around line 750-757: The returned token dict currently maps "refresh_token" to
api_refresh_token which breaks INTL refresh flow; change the mapping so
"refresh_token" uses self.intl_refresh_token (and include "intl_refresh_token":
self.intl_refresh_token) while keeping "api_refresh_token": api_refresh_token
and "api_access_token": api_access_token; update the return that builds
{"access_token": api_access_token, "refresh_token": ..., "api_access_token":
..., "api_refresh_token": ..., "api_user_id": api_user_id, "expires_at":
expires_at} to use self.intl_refresh_token for the refresh_token and add the
intl_refresh_token key, and make the same corresponding change for the duplicate
token-returning block later in the file that uses the same variables
(api_access_token, api_refresh_token, api_user_id, expires_at).
---
Nitpick comments:
In `@pysmarthashtag/api/authentication.py`:
- Around line 678-679: Change the bare re-raises of parsing exceptions to chain
the original exception using "raise ... from e": in the except blocks (e.g., the
block that catches (KeyError, ValueError, TypeError) and raises
SmartAPIError("Could not parse INTL login response: {e}")), update the raise to
use "raise SmartAPIError(...) from e". Apply the same pattern for the other
similar except blocks referenced in the file (the ones at the locations
corresponding to lines around 711 and 748) so all SmartAPIError raises preserve
the original exception chain.
- Around line 549-550: Move the local imports of time and uuid out of the
methods and into the module-level imports at the top of the file (add "import
time" and "import uuid" alongside the other imports), then remove the
in-function "import time" and "import uuid" lines from _generate_intl_headers
and _generate_vehicle_api_headers so both functions use the module-level
symbols; verify there are no other local import duplicates and run tests to
ensure no naming conflicts.
|
|
||
| return headers | ||
|
|
||
| def _generate_vehicle_api_headers(self, body: str = None) -> dict[str, str]: |
Contributor
There was a problem hiding this comment.
Fix implicit Optional type hint.
PEP 484 prohibits implicit Optional. The parameter body: str = None should use explicit Optional[str].
🔧 Proposed fix
- def _generate_vehicle_api_headers(self, body: str = None) -> dict[str, str]:
+ def _generate_vehicle_api_headers(self, body: Optional[str] = None) -> dict[str, str]:📝 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.
Suggested change
| def _generate_vehicle_api_headers(self, body: str = None) -> dict[str, str]: | |
| def _generate_vehicle_api_headers(self, body: Optional[str] = None) -> dict[str, str]: |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 579-579: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysmarthashtag/api/authentication.py` at line 579, The parameter annotation
in _generate_vehicle_api_headers currently uses an implicit Optional via body:
str = None; change the signature to explicitly declare Optional[str] (e.g.,
body: Optional[str] = None) and add the corresponding typing import (from typing
import Optional) or use the explicit union syntax (str | None) if the project
targets Python 3.10+; update any references or type checks in the function if
needed to match the new annotation.
onpremcloudguy
force-pushed
the
onpremcloudguy-working_Intl_EU
branch
from
3f4a333 to
a11cbb2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
the previous version you committed didn't work for Intl, I have completed testing via VPN's in the EU and Japan and all tests have passed.
Add support for Hello Smart International (INTL) API used in Australia, Singapore, Israel, and other international markets.
Summary by CodeRabbit
New Features
Improvements