Add get_electric_bill_history and get_gas_bill_history via business portal silent SSO#6
Merged
Merged
Conversation
…ortal Adds two new typed client methods that fetch per-billing-period detail from the National Grid business portal API (accountservice-cu-mba-exp), which returns fields not available through the standard GraphQL bills endpoint: utility/supplier charge breakdown, total kWh/therms, average daily usage, and demand data (TOU, peak kW) for electric. Authentication uses a prompt=none silent SSO flow: after the consumer portal OIDC login populates B2C session cookies on the shared aiohttp session, the business portal authorize request redirects immediately without any login UI. The auth code is captured from the Location header (before following the redirect) and exchanged for a token with the correct audience (aud=a26ad492). Also adds: - get_premise() for address-based premise/meter lookup (public, no auth) - NationalGridBusinessAuth with correct tenant path (loginnationalgridus.onmicrosoft.com) - _get_auth_silent() in oidchelper for prompt=none redirect capture - extra_auth_params support threaded through async_auth_oidc/_get_auth - ElectricBillRecord and GasBillRecord TypedDicts in models.py - Extractors for both bill history response envelopes - examples/bill-history.py and examples/premise-lookup.py - Cleanup: removed dead _get_id_token method, _id_token field, LoginData.refresh_token Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Add unit tests covering all previously uncovered branches introduced in the
bill history PR:
- _request_business_rest: success path, 4xx/5xx raises RestAPIError, and the
inner response.text() failure case (sets response_text=None)
- _get_business_id_token: cache hit, no-credentials early return, exception
from async_login, None access token, successful caching, missing id_token,
and the double-checked locking inner cache hit (concurrent refresh simulation)
- NationalGridBusinessAuth.async_login: verifies it delegates to async_auth_oidc
with extra_auth_params={"prompt": "none"}
- extractors: electricBillHistory/gasBillHistory inner-value-not-list branches
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GraphQL premise lookup and business-portal electric/gas billing history APIs; extends OIDC to return id_token and support silent SSO; adds TypedDict models, query/request builders, extractors, client helpers (business REST, headers, token cache), example scripts, docs updates, and comprehensive tests. ChangesPremise Lookup and Bill History Features
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
src/py_nationalgrid/auth.py (1)
91-122: 💤 Low valueUnused
username/passwordparameters in silent SSO flow may mislead callers.When
prompt=none, the_get_auth_silentpath is taken and credentials are never used—B2C relies solely on session cookies. The method signature requiring these parameters could confuse callers into thinking credentials are validated during silent SSO.Consider documenting this explicitly in the docstring or accepting empty strings as valid inputs for the silent case.
🤖 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 `@src/py_nationalgrid/auth.py` around lines 91 - 122, The async_login method currently requires username and password although the silent SSO path uses prompt=none and ignores credentials; update the function to avoid misleading callers by either (A) changing the signature of async_login to accept username: str | None and password: str | None (or provide default None) and propagate those values into the async_auth_oidc call, or (B) keep the signature but explicitly document in the async_login docstring that username/password are unused for silent SSO (prompt=none) and callers may pass None or empty strings; mention async_login, async_auth_oidc and the prompt=none extra_auth_params so reviewers can locate the relevant code to update.src/py_nationalgrid/client.py (1)
1618-1624: 💤 Low valueRequests proceed with empty bearer token when authentication fails.
When
_get_business_id_token()returnsNone(e.g., missing credentials or SSO failure), the code usesid_token or ""to construct an empty bearer token. The subsequent request will fail with a 401, but the error won't clearly indicate that authentication was the root cause.The docstrings state "Requires authentication" — consider failing early with a clear error when
id_tokenisNoneto improve debuggability.♻️ Example early-exit pattern
async def get_electric_bill_history(...) -> list[ElectricBillRecord]: ... id_token = await self._get_business_id_token() + if not id_token: + raise RestAPIError( + "Business portal authentication required but id_token is unavailable", + url="", + method="POST", + ) rest_request = electric_bill_history_request(...) - business_headers = self._build_business_headers(id_token or "") + business_headers = self._build_business_headers(id_token)Apply the same pattern to
get_gas_bill_history.Also applies to: 1664-1670
🤖 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 `@src/py_nationalgrid/client.py` around lines 1618 - 1624, The code currently continues with an empty bearer token when _get_business_id_token() returns None, causing opaque 401s; update the call sites (electric_bill_history_request path and likewise in get_gas_bill_history) to fail early when id_token is None by raising a clear exception (e.g., AuthenticationError or ValueError) before calling _build_business_headers(id_token) — locate uses of _get_business_id_token, check the id_token result, and raise with a descriptive message like "Missing business ID token: authentication required" so callers get an explicit authentication error instead of proceeding with an empty token.
🤖 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 `@src/py_nationalgrid/client.py`:
- Around line 678-680: The current assignment to self._business_token_expires_at
uses expires_in without guarding for None, which can raise TypeError if id_token
is truthy but expires_in is None; update the logic in the method where id_token
and expires_in are handled (the block that sets self._business_id_token and
self._business_token_expires_at) to mirror the consumer auth pattern by
requiring both id_token and expires_in truthy before computing
self._business_token_expires_at (e.g., only set self._business_token_expires_at
= time.time() + expires_in when expires_in is not None), otherwise set or leave
expires_at as None/unchanged.
In `@src/py_nationalgrid/rest_queries.py`:
- Line 15: The BUSINESS_SUBSCRIPTION_KEY constant is hard-coded; remove the
literal and instead load it from a runtime secret source (e.g., environment
variable or secret manager) where it is required, e.g., replace usages of
BUSINESS_SUBSCRIPTION_KEY with a getter that reads
os.environ.get("BUSINESS_SUBSCRIPTION_KEY") (or a secret client) and raises a
clear error if missing; update any functions that reference
BUSINESS_SUBSCRIPTION_KEY in this module (rest_queries.py) to use that getter
and ensure tests/configs set the env var for CI and local runs.
---
Nitpick comments:
In `@src/py_nationalgrid/auth.py`:
- Around line 91-122: The async_login method currently requires username and
password although the silent SSO path uses prompt=none and ignores credentials;
update the function to avoid misleading callers by either (A) changing the
signature of async_login to accept username: str | None and password: str | None
(or provide default None) and propagate those values into the async_auth_oidc
call, or (B) keep the signature but explicitly document in the async_login
docstring that username/password are unused for silent SSO (prompt=none) and
callers may pass None or empty strings; mention async_login, async_auth_oidc and
the prompt=none extra_auth_params so reviewers can locate the relevant code to
update.
In `@src/py_nationalgrid/client.py`:
- Around line 1618-1624: The code currently continues with an empty bearer token
when _get_business_id_token() returns None, causing opaque 401s; update the call
sites (electric_bill_history_request path and likewise in get_gas_bill_history)
to fail early when id_token is None by raising a clear exception (e.g.,
AuthenticationError or ValueError) before calling
_build_business_headers(id_token) — locate uses of _get_business_id_token, check
the id_token result, and raise with a descriptive message like "Missing business
ID token: authentication required" so callers get an explicit authentication
error instead of proceeding with an empty token.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 06c1685b-b59b-4c6c-8def-152b1d21702c
📒 Files selected for processing (19)
.gitignoreCLAUDE.mdREADME.mdexamples/bill-history.pyexamples/premise-lookup.pypyproject.tomlsrc/py_nationalgrid/__init__.pysrc/py_nationalgrid/auth.pysrc/py_nationalgrid/client.pysrc/py_nationalgrid/extractors.pysrc/py_nationalgrid/models.pysrc/py_nationalgrid/oidchelper.pysrc/py_nationalgrid/queries.pysrc/py_nationalgrid/rest_queries.pytests/test_client.pytests/test_coverage.pytests/test_interval_reads.pytests/test_retry.pytests/test_typed_client.py
Comment on lines
+678
to
+680
| if id_token: | ||
| self._business_id_token = id_token | ||
| self._business_token_expires_at = time.time() + expires_in |
There was a problem hiding this comment.
Potential TypeError if expires_in is None while id_token is truthy.
The condition on line 678 only checks if id_token:, but line 680 performs arithmetic with expires_in. If the auth returns a valid id_token but None for expires_in, this will raise TypeError: unsupported operand type(s) for +: 'float' and 'NoneType'.
The consumer auth pattern (lines 625–631) guards against this by checking if token and expires_in:.
🐛 Proposed fix
- if id_token:
+ if id_token and expires_in:
self._business_id_token = id_token
self._business_token_expires_at = time.time() + expires_in
logger.debug("Business id_token obtained, expires in %d seconds", expires_in)
else:
- logger.error("Business portal silent SSO produced no id_token")
+ logger.error("Business portal silent SSO produced no id_token or expires_in")🤖 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 `@src/py_nationalgrid/client.py` around lines 678 - 680, The current assignment
to self._business_token_expires_at uses expires_in without guarding for None,
which can raise TypeError if id_token is truthy but expires_in is None; update
the logic in the method where id_token and expires_in are handled (the block
that sets self._business_id_token and self._business_token_expires_at) to mirror
the consumer auth pattern by requiring both id_token and expires_in truthy
before computing self._business_token_expires_at (e.g., only set
self._business_token_expires_at = time.time() + expires_in when expires_in is
not None), otherwise set or leave expires_at as None/unchanged.
|
|
||
| # Business portal (accountservice-cu-mba-exp) — uses idToken auth | ||
| BUSINESS_BASE_URL = "https://gridapi-cm-prod-appgw.dpit.nationalgrid.com/api" | ||
| BUSINESS_SUBSCRIPTION_KEY = "f1098e143d4c4d5b81eb0a86667d0ddf" |
There was a problem hiding this comment.
Move the business subscription key out of source.
Hard-coding this key in code exposes credentials and makes rotation brittle. Pass it via runtime config/secret management instead of a committed literal.
🧰 Tools
🪛 Betterleaks (1.2.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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py_nationalgrid/rest_queries.py` at line 15, The
BUSINESS_SUBSCRIPTION_KEY constant is hard-coded; remove the literal and instead
load it from a runtime secret source (e.g., environment variable or secret
manager) where it is required, e.g., replace usages of BUSINESS_SUBSCRIPTION_KEY
with a getter that reads os.environ.get("BUSINESS_SUBSCRIPTION_KEY") (or a
secret client) and raises a clear error if missing; update any functions that
reference BUSINESS_SUBSCRIPTION_KEY in this module (rest_queries.py) to use that
getter and ensure tests/configs set the env var for CI and local runs.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @virtitnerd. The following files were modified: * `examples/bill-history.py` * `examples/premise-lookup.py` * `src/py_nationalgrid/auth.py` * `src/py_nationalgrid/client.py` * `src/py_nationalgrid/extractors.py` * `src/py_nationalgrid/oidchelper.py` * `src/py_nationalgrid/queries.py` * `src/py_nationalgrid/rest_queries.py` * `tests/test_client.py` * `tests/test_coverage.py` * `tests/test_retry.py` * `tests/test_typed_client.py` These files were kept as they were: * `tests/test_interval_reads.py` These file types are not supported: * `.gitignore` * `CLAUDE.md` * `README.md` * `pyproject.toml`
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@examples/bill-history.py`:
- Around line 48-52: The docstring for the main() function is exceeding the max
line length (Ruff E501); rewrap the docstring text to shorter lines (e.g., 79
characters) so each line is within the lint limit, preserving the same content
and punctuation. Locate the multi-line triple-quoted string inside main() and
split long sentences into multiple lines or paragraphs while keeping
descriptions of behavior (CLI steps, printed summaries, per-fuel history
details, and no-linked-accounts message) intact.
In `@examples/premise-lookup.py`:
- Around line 19-32: The docstrings use tab characters and have overlong lines
causing E101/E501; update the docstring(s) for the argument-parsing function
(parse_args) and the subsequent docstring block to use spaces for indentation
and wrap lines to a safe length (e.g., 79 or 88 chars), reformat the
Parameters/Returns bullets so each line is wrapped and indented consistently (4
spaces) and ensure no tab characters remain; keep content the same but split
long sentences into shorter lines and align the hyphenated list items under the
same indentation level.
In `@src/py_nationalgrid/auth.py`:
- Around line 44-54: The docstring for the authentication function (the auth
method in src/py_nationalgrid/auth.py) contains lines that exceed the configured
max line length and trigger Ruff E501; shorten or wrap the long sentences (for
example the summary, the "Performs the OIDC..." sentence, the Parameters and
Returns descriptions) so each line fits the project's line-length limit (or
rephrase to be more concise), and apply the same fix to the second docstring
block around lines 106-116; keep the content the same but break sentences or
shorten phrases to meet the linter limit.
In `@src/py_nationalgrid/client.py`:
- Around line 140-149: The docstring blocks for the NationalGridClient
initializer are exceeding the project's 100-character line limit and causing
Ruff failures; reflow each docstring/comment to wrap at or below 100 characters
per line (preserve existing content and sentence breaks) in the
NationalGridClient.__init__ docstring and the other noted docstring blocks
(lines 267-281, 419-434, 481-498, 623-630, 665-672, 718-733, 748-755, 779-791,
1708-1725), ensuring parameter and description sections remain clear and aligned
with the project's docstring style.
In `@src/py_nationalgrid/extractors.py`:
- Around line 511-519: The docstrings in src/py_nationalgrid/extractors.py
(including the one starting "Extracts collection arrangement records..." and the
other docstrings at the ranges mentioned) exceed the project's line-length limit
and fail Ruff; reflow/wrap each docstring to respect the repository's max line
length (e.g., 88 chars) so each line is within the limit, preserving all text
and indentation and keeping the same triple-quoted docstring and paragraphs,
especially for the descriptions and the "Returns:" / "Raises:" sections in the
functions/methods such as the collection arrangements extractor and the other
extractors referenced at 545-555, 581-589, and 645-656. Ensure no code or markup
is changed, only the docstring line breaks.
In `@src/py_nationalgrid/oidchelper.py`:
- Around line 230-245: Reflow the long docstring blocks in
src/py_nationalgrid/oidchelper.py (the docstring that begins "Retrieve the
OpenID Connect discovery document..." and the other long docstrings around the
Parameters/Returns sections) so no line exceeds 100 characters; wrap
descriptions for Parameters, Returns, and Raises to 100-char width or less while
preserving the same text and formatting (indentation and parameter names).
Locate the docstring attached to the OpenID discovery retrieval function in
oidchelper.py and the other long docstrings referenced (the blocks around the
Parameters/Returns sections) and break long lines into shorter sentences/lines
to satisfy Ruff's line-length rule.
In `@src/py_nationalgrid/queries.py`:
- Around line 617-627: The docstrings around the "Builds a collection
arrangements GraphQL request" function contain mixed tabs/spaces and lines
exceeding 100 chars causing Ruff E101/E501; normalize indentation to spaces only
and reflow long lines to <=100 characters (prefer ~72 for summary and ~100 for
body), applying the same fixes to the other docstrings referenced at the regions
corresponding to 658-670 and 683-691; ensure triple-quoted strings keep
consistent indentation, wrap parameter/return descriptions to shorter lines, and
remove any embedded tabs so Ruff no longer flags E101/E501.
In `@src/py_nationalgrid/rest_queries.py`:
- Around line 35-45: Docstrings for the REST request builders (the GET
RestRequest that mentions startDateTime / start_datetime and RestRequest)
contain mixed tabs/spaces and overlong lines triggering Ruff E101/E501; fix by
normalizing indentation to spaces only, rewrapping long lines to a safe length
(e.g., <=88 chars), and ensuring consistent triple-quoted docstring formatting
for all similar builders (the blocks flagged at 35-45, 72-83, 100-110, 124-134,
143-151) so they use only spaces and no lines exceed the configured max line
length.
In `@tests/test_client.py`:
- Around line 110-119: The docstring for the OIDC test helper (the multi-line
docstring beginning "Test helper that simulates an OIDC login...") exceeds the
configured line-length (Ruff E501); reflow/wrap the text so each line is under
the project's limit (break at sentence or clause boundaries and keep
descriptions concise), and apply the same wrapping to the other expanded
test-helper docstrings in this file that currently exceed line length. Ensure
you only change docstring formatting (no logic) and preserve the same content
and parameter/return wording.
In `@tests/test_coverage.py`:
- Around line 421-426: The docstring return description exceeds the
100-character line limit; reflow the Returns section in this helper so each line
is under 100 chars (wrap the tuple description and the field explanations),
e.g., break the sentence describing the 3-tuple into multiple shorter lines so
the access_token/id_token/expires_in_seconds parts each fit; apply the same
wrapping style to the other helper docstrings referenced at the other
occurrences (the blocks around lines 469-474 and 630-635) so Ruff E501 is
resolved.
In `@tests/test_retry.py`:
- Around line 71-83: Shorten long docstring lines in tests/test_retry.py
(notably the docstring that starts "Return a fixed authentication token triple
used by tests." which documents the returned tuple `(access_token, id_token,
expires_in)` with values "token", "id-tok", 3600) and the other long docstrings
flagged by the reviewer by wrapping sentences so no line exceeds the linter
limit (e.g., 88 chars), preserving the exact content and wording but splitting
long sentences and parameter/return descriptions across multiple shorter lines;
apply the same wrapping/fix to the other test helper/docstrings mentioned in the
review so Ruff E501 errors are resolved.
In `@tests/test_typed_client.py`:
- Around line 2176-2181: Reflow the long test docstrings in
tests/test_typed_client.py so no line exceeds 100 characters: find the docstring
that begins "Verify that get_premise parses and returns premise nodes with
nested meter data." and the other long docstring referenced (the one around the
later test), and wrap the text into multiple lines under the project's 100-char
limit while preserving the same wording and triple-quote delimiters for the test
functions that assert get_premise returns premise fields and nested meter nodes
(e.g., premiseNumber, premiseStatus, compressedAddress, fuelType, meterNumber).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ec52024-70f4-4963-801e-9cfac20a7446
📒 Files selected for processing (12)
examples/bill-history.pyexamples/premise-lookup.pysrc/py_nationalgrid/auth.pysrc/py_nationalgrid/client.pysrc/py_nationalgrid/extractors.pysrc/py_nationalgrid/oidchelper.pysrc/py_nationalgrid/queries.pysrc/py_nationalgrid/rest_queries.pytests/test_client.pytests/test_coverage.pytests/test_retry.pytests/test_typed_client.py
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 12 file(s) based on 12 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 12 file(s) based on 12 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/py_nationalgrid/oidchelper.py (1)
290-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle request timeouts in silent-auth errors explicitly.
File: src/py_nationalgrid/oidchelper.py (lines 290-301)
_get_auth_silent()catches onlyaiohttp.ClientError, butaiohttp.ClientTimeout(total=...)timeouts raiseasyncio.TimeoutError(aiohttp timeout exceptions inherit fromasyncio.TimeoutError), so timeouts can escape instead of being normalized toCannotConnectError.
_fetch()uses the sameaiohttp.ClientTimeout(total=timeout)pattern but also catches onlyaiohttp.ClientError, so apply the same timeout normalization there too.Suggested fix
@@ - except aiohttp.ClientError as err: + except (aiohttp.ClientError, asyncio.TimeoutError) as err: raise CannotConnectError(f"Silent auth network error: {err}") from err🤖 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 `@src/py_nationalgrid/oidchelper.py` around lines 290 - 301, Both _get_auth_silent() and _fetch() currently only catch aiohttp.ClientError around their aiohttp requests using aiohttp.ClientTimeout, but asyncio.TimeoutError (raised by aiohttp timeouts) can escape; update the exception handling in the async with session.get(...) / session.request(...) blocks to also catch asyncio.TimeoutError (or Exception as needed) and normalize it to raise CannotConnectError with the timeout error included (same pattern as catching aiohttp.ClientError), ensuring the session.get/session.request calls that use aiohttp.ClientTimeout(total=timeout) are wrapped by a try/except that raises CannotConnectError on both aiohttp.ClientError and asyncio.TimeoutError so timeouts are handled consistently.
♻️ Duplicate comments (1)
src/py_nationalgrid/client.py (1)
753-760:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
expires_inbefore computing business token expiry.At Line 756,
time.time() + expires_incan raiseTypeErrorwhen silent SSO returns anid_tokenwith a missing expiry value.Proposed fix
- _, id_token, expires_in = result - if id_token: + _, id_token, expires_in = result + if id_token and expires_in: self._business_id_token = id_token self._business_token_expires_at = time.time() + expires_in logger.debug("Business id_token obtained, expires in %d seconds", expires_in) else: - logger.error("Business portal silent SSO produced no id_token") + logger.error("Business portal silent SSO produced no id_token or expires_in") + self._business_id_token = None + self._business_token_expires_at = None🤖 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 `@src/py_nationalgrid/client.py` around lines 753 - 760, The code assigns self._business_token_expires_at = time.time() + expires_in without validating expires_in; update the block that handles result (the variables id_token and expires_in) to guard expires_in before computing expiry: when id_token is present, verify expires_in is a valid number (int/float and not None) and only then set self._business_token_expires_at = time.time() + expires_in, otherwise log a warning/error and either set a sensible default TTL or leave _business_token_expires_at unset/None; keep the existing assignment to self._business_id_token and the logger.debug/ logger.error paths but add the expires_in validation to avoid TypeError.
🤖 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.
Outside diff comments:
In `@src/py_nationalgrid/oidchelper.py`:
- Around line 290-301: Both _get_auth_silent() and _fetch() currently only catch
aiohttp.ClientError around their aiohttp requests using aiohttp.ClientTimeout,
but asyncio.TimeoutError (raised by aiohttp timeouts) can escape; update the
exception handling in the async with session.get(...) / session.request(...)
blocks to also catch asyncio.TimeoutError (or Exception as needed) and normalize
it to raise CannotConnectError with the timeout error included (same pattern as
catching aiohttp.ClientError), ensuring the session.get/session.request calls
that use aiohttp.ClientTimeout(total=timeout) are wrapped by a try/except that
raises CannotConnectError on both aiohttp.ClientError and asyncio.TimeoutError
so timeouts are handled consistently.
---
Duplicate comments:
In `@src/py_nationalgrid/client.py`:
- Around line 753-760: The code assigns self._business_token_expires_at =
time.time() + expires_in without validating expires_in; update the block that
handles result (the variables id_token and expires_in) to guard expires_in
before computing expiry: when id_token is present, verify expires_in is a valid
number (int/float and not None) and only then set
self._business_token_expires_at = time.time() + expires_in, otherwise log a
warning/error and either set a sensible default TTL or leave
_business_token_expires_at unset/None; keep the existing assignment to
self._business_id_token and the logger.debug/ logger.error paths but add the
expires_in validation to avoid TypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0abb5b1f-d975-45c8-ac0d-d9a032937652
📒 Files selected for processing (12)
examples/bill-history.pyexamples/premise-lookup.pysrc/py_nationalgrid/auth.pysrc/py_nationalgrid/client.pysrc/py_nationalgrid/extractors.pysrc/py_nationalgrid/oidchelper.pysrc/py_nationalgrid/queries.pysrc/py_nationalgrid/rest_queries.pytests/test_client.pytests/test_coverage.pytests/test_retry.pytests/test_typed_client.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
asyncio.TimeoutError (aliased as builtin TimeoutError in Python 3.11+) is not a subclass of aiohttp.ClientError, so aiohttp.ClientTimeout expiry could escape uncaught. Both _get_auth_silent and _fetch now catch (aiohttp.ClientError, TimeoutError) and normalize to CannotConnectError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/py_nationalgrid/oidchelper.py (2)
369-370:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock overrides of core OAuth parameters.
auth_params.update(extra_auth_params)lets callers replaceclient_id,redirect_uri,response_type,scope, or the PKCE fields, while_get_access()still exchanges the code with the original arguments. One accidental override here is enough to make the authorize/token pair inconsistent.🛡️ Proposed fix
if extra_auth_params: - auth_params.update(extra_auth_params) + reserved = { + "client_id", + "response_type", + "redirect_uri", + "scope", + "code_challenge", + "code_challenge_method", + } + conflicting = reserved & extra_auth_params.keys() + if conflicting: + raise ValueError( + f"extra_auth_params cannot override reserved auth params: {sorted(conflicting)}" + ) + auth_params.update(extra_auth_params)🤖 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 `@src/py_nationalgrid/oidchelper.py` around lines 369 - 370, The current auth_params.update(extra_auth_params) in the auth flow allows callers to override core OAuth fields and break the authorize/token consistency; instead, in the function where auth_params and extra_auth_params are merged (the block that currently calls auth_params.update(extra_auth_params) and the related _get_access exchange), filter extra_auth_params to remove any core OAuth keys before merging — e.g., disallow keys like client_id, redirect_uri, response_type, scope, code, code_verifier, grant_type and any PKCE-related fields — then update auth_params only with the remaining safe extras so the original parameters used by _get_access remain intact.
160-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare
id_tokeninTokenDictto matchasync_auth_oidc()
async_auth_oidc()readstokens.get("id_token", "")and returns(access_token, id_token, expires_in), butTokenDictomitsid_token(src/py_nationalgrid/oidchelper.py lines ~160-161 and 190-195), leaving the TypedDict typing inconsistent.♻️ Proposed fix
class TokenDict(TypedDict, total=False): """Dictionary to store OAuth tokens.""" access_token: str + id_token: str expires_in: int # Token lifetime in seconds🤖 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 `@src/py_nationalgrid/oidchelper.py` around lines 160 - 161, TokenDict omits the id_token key while async_auth_oidc() and code that does tokens.get("id_token", "") expect it; update the TokenDict TypedDict to include an optional/NotRequired "id_token" of type str (or Optional[str]) so the typing matches usage, and ensure any places that construct the tokens dict (the code that assigns access_token/id_token) populate id_token consistently with that type.
🤖 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.
Outside diff comments:
In `@src/py_nationalgrid/oidchelper.py`:
- Around line 369-370: The current auth_params.update(extra_auth_params) in the
auth flow allows callers to override core OAuth fields and break the
authorize/token consistency; instead, in the function where auth_params and
extra_auth_params are merged (the block that currently calls
auth_params.update(extra_auth_params) and the related _get_access exchange),
filter extra_auth_params to remove any core OAuth keys before merging — e.g.,
disallow keys like client_id, redirect_uri, response_type, scope, code,
code_verifier, grant_type and any PKCE-related fields — then update auth_params
only with the remaining safe extras so the original parameters used by
_get_access remain intact.
- Around line 160-161: TokenDict omits the id_token key while async_auth_oidc()
and code that does tokens.get("id_token", "") expect it; update the TokenDict
TypedDict to include an optional/NotRequired "id_token" of type str (or
Optional[str]) so the typing matches usage, and ensure any places that construct
the tokens dict (the code that assigns access_token/id_token) populate id_token
consistently with that type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd9208f4-ed6c-460b-99c7-8db446b35f55
📒 Files selected for processing (1)
src/py_nationalgrid/oidchelper.py
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.
Summary
get_electric_bill_history(account_number, customer_number)andget_gas_bill_history(account_number, customer_number)typed methods returninglist[ElectricBillRecord]/list[GasBillRecord]accountservice-cu-mba-exp) which has fields not available in the standard GraphQL bills endpoint: utility/supplier charge breakdown, total kWh/therms, avg daily usage, and demand data (TOU, peak kW) for electricget_premise()for address-based premise/meter lookup (public endpoint, no auth required)Authentication approach
The business portal validates that the Bearer
id_tokenhasaud=a26ad492(a different client registration from the consumer portal). HAR analysis confirmed the browser silently re-authenticates after consumer portal login via Azure AD B2C SSO cookies.Implementation uses
prompt=nonesilent SSO:aiohttpsessionprompt=nonegets an immediate 302 redirect — no login UILocationheader (before following the redirect) and exchanged for a token with the correctaudThis required:
_get_auth_silent()helper inoidchelper.py(non-redirecting GET, readsLocationheader)extra_auth_paramsthreaded throughasync_auth_oidc/_get_authNationalGridBusinessAuthwith the correct tenant path (loginnationalgridus.onmicrosoft.com)_request_business_restin client bypassingbuild_headersto avoid subscription key collisionsCleanup
_get_id_token()method and_id_tokenfield (set but never read after business portal switched to its own token)LoginData.refresh_token(was populated during an earlier failed cross-client refresh approach; never read)Test plan
uv run pytest— 202 tests pass, 96% coverageuv run python examples/bill-history.py --username ... --password ...— returns electric and gas bill recordsuv run python examples/premise-lookup.py --street-name ... --city ... --state NY --zip ...— returns premise/meter info🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests