chore: retro-review PR #86 — feat(providers): add OpenElectricity wholesale-price client (Phase 7 PR-2)

[Artic0din]

Synthetic retro-review of merged PR #86. Diff = original bdf6f74..40fff3e (same as original PR). Do NOT merge; close after @claude review posts.

[Artic0din]

@claude review

[claude]

Claude finished @Artic0din's task in 16m 14s —— View job

---

Code Review — PR #86: feat(providers): add OpenElectricity wholesale-price client (Phase 7 PR-2)

Tasks:

• Gather context and read changed files
• Review openelectricity.py provider implementation
• Review test_openelectricity_provider.py test suite
• Review manifest.json, conftest.py, CHANGELOG.md, DECISIONS.md changes
• Post final review

---

Overall Assessment

This is a well-engineered standalone module. The audit invariants are explicit in the docstring, the security properties (key redaction, timeout bounding, lazy import) are correct, and the test suite covers every named AC. A few actionable findings below.

---

Findings

🔴 Bug — _is_auth_error() "forbidden" string match produces false positives

File: custom_components/pricehawk/providers/openelectricity.py:199-202

if (
    "401" in msg
    or "unauthor" in msg
    or "invalid api key" in msg
    or "forbidden" in msg   # ← too broad
):

"forbidden" as a substring match will fire on network and TLS errors whose message text uses that word in non-HTTP-401 context (e.g. "SSL verify failed — all connections forbidden by network policy", corporate proxy errors, DNS rebind protection messages). That would raise ConfigEntryAuthFailed and prompt the user to re-enter their API key when the real problem is connectivity — confusing and wrong.

HTTP 403 Forbidden is also semantically different from 401 Unauthorized: a valid key with insufficient plan tier returns 403, and re-entering the key won't fix it. Treating 403 as an auth failure sends the user on a wild goose chase.

Suggested fix: drop the string "forbidden" from the message check and keep only the class-name check for "forbidden" (which will only fire on an exception class literally named Forbidden*). Alternatively, tighten to "403" in msg or "forbidden" in msg only when paired with "http" to anchor it to an HTTP context.

Fix this →

---

🟡 Missing test — region mismatch in _extract_latest_for_region

File: tests/test_openelectricity_provider.py

There is no test for the case where the API returns data for a different region than was requested. _extract_latest_for_region filters by row_region != region at line 250, so a response containing NSW1 data when VIC1 was requested should return None. This path is untested. Given that the correctness of result attribution depends on this filter working, it's worth a targeted test.

Fix this →

---

🟡 network_code type mismatch suppressed with # type: ignore

File: custom_components/pricehawk/providers/openelectricity.py:137

network_code=network_code,  # type: ignore[arg-type]

This suppression suggests the SDK expects a typed enum or Literal["NEM", "WEM", "AU"] but receives a plain str. If the SDK ever does strict runtime validation on this parameter (e.g. uses isinstance(arg, NetworkCode)), this will fail silently. Worth checking the SDK's actual parameter type: if it exposes a NetworkCode or similar enum, prefer using it here. If it truly accepts str, the ignore is unnecessary noise and should be removed.

---

🟡 Happy-path test doesn't verify cache is populated

File: tests/test_openelectricity_provider.py:117-143

test_happy_path_nem_region_returns_wholesale_price_with_attribution doesn't assert that src.last_good("NSW1") equals the returned result after a successful fetch. The cache-write path is exercised implicitly by test_non_auth_error_returns_none_and_preserves_cache, but making the happy-path assertion explicit would prevent a regression where last_good silently stops being set on success.

---

🟢 Minor — _mock_async_client branch for AsyncMock is implicit

File: tests/test_openelectricity_provider.py:90-95

The condition callable(get_market_result) and not isinstance(get_market_result, AsyncMock) silently treats an AsyncMock argument as a return_value rather than a side_effect. No current test passes an AsyncMock directly, so this branch is dead code, but it could confuse a future contributor. A brief inline comment explaining the exclusion would help.

---

Positive observations

• Module-level audit invariant docstring (lines 8–15) — makes the security contract reviewable at a glance. Excellent practice.
• WholesalePrice as frozen=True, slots=True — correct cross-PR stability contract; consumers can safely cache instances.
• _scrub() on every _LOGGER.warning call that uses str(exc) — CWE-532 compliance is thorough, including the leaky-exception test at test_log_scrubs_api_key_from_sdk_error.
• asyncio.timeout with a Final constant — right pattern; no magic numbers.
• _extract_latest_for_region using getattr duck-typing with a max() on timestamp — robust against SDK shape drift and correctly handles multiple interval points by picking the latest.
• conftest.py ConfigEntryAuthFailed addition — wired correctly with parent-module attribute linkage; follows existing Phase 3.0c pattern exactly.
• DECISIONS.md entries — D-P7-5 through D-P7-8 are well-scoped with explicit alternatives-considered sections. Good architectural record.

---

The "forbidden" false positive (🔴) is the only finding worth a fix before this module is wired into the coordinator in 07-02b. The two 🟡 test gaps are low-risk given the existing coverage depth.

[claude]

Claude finished @Artic0din's task in 13m 19s —— View job

---

Code Review — PR #86: feat(providers): add OpenElectricity wholesale-price client

• Gather context and understand request
• Read changed files
• Review openelectricity.py provider
• Review test_openelectricity_provider.py
• Review supporting changes (manifest, conftest, changelog, decisions)
• Post findings

---

Overall this is a well-structured, defensively-written provider module. The audit-invariant approach (M1/M2/M3 tagged inline), the frozen dataclass cross-PR contract, and the scrubbing layer are all solid patterns. The test file maps cleanly to acceptance criteria and the SDK-stub approach avoids a real network dependency. Three correctness concerns worth addressing before the wiring PR (07-02b).

---

Correctness issues

1. _is_auth_error() maps HTTP 403 to ConfigEntryAuthFailed — wrong HA signal [openelectricity.py:197-208]

"forbidden" in the message (and "forbidden" in the class name) causes a 403 to raise ConfigEntryAuthFailed, which triggers HA's "re-enter credentials" repair flow. A 403 usually means the key is valid but lacks the required plan tier or region entitlement — the user's API key is not wrong, it just doesn't have permission. Prompting for a new key is misleading and would loop forever.

# Current — 403 falls through to ConfigEntryAuthFailed
if (
    "401" in msg
    or "unauthor" in msg
    or "invalid api key" in msg
    or "forbidden" in msg   # <-- 403 ≠ 401
):
    return True

Suggested fix: strip "forbidden" from the message check and from the class-name check. If the SDK exposes a specific ForbiddenError class, handle it separately with a WARNING (not ConfigEntryAuthFailed). Fix this →

---

2. _scrub() 8-char prefix replacement can produce false-positive redactions [openelectricity.py:101-103]

if len(self._api_key) >= 8:
    scrubbed = scrubbed.replace(self._api_key[:8], _REDACTED_PREFIX)

For a key like "sk-test-12345678" the prefix is "sk-test-". That substring could appear legitimately in URLs (/v4/sk-test-networks), library names, or other log text, silently mangling unrelated log lines. The prefix scrub was presumably added to catch partial key leaks, but an 8-char window is too short and too likely to collide. A safer floor is 16 chars, or drop the prefix replacement entirely (the full-key replacement on the line above already covers the common case). Fix this →

---

3. float(val) silently accepts NaN and inf from the API [openelectricity.py:259]

candidates.append((float(val), ts))

The OpenElectricity API can return null or sentinel values during network outages that the SDK may surface as float('nan') or float('inf'). These pass through float() without error, get selected by max(), and end up in WholesalePrice.price_aud_per_mwh. Downstream cost-calculation sensors would then produce nan entity states, which HA renders as unknown with no diagnostic signal.

# Suggested guard
val_f = float(val)
if not math.isfinite(val_f):
    continue
candidates.append((val_f, ts))

Fix this →

---

Test gaps

4. The max() timestamp-selection path is never exercised

_build_response always creates a single data point. No test constructs a response with multiple timestamps to verify that max(candidates, key=lambda pair: pair[1]) picks the right one. A regression on the ordering logic would pass all current tests.

5. Timezone-naive timestamp fallback (ts.replace(tzinfo=timezone.utc)) is untested [openelectricity.py:258]

There's a guard for naive timestamps from the SDK but no test that exercises the branch. Worth one test with datetime(2026, 5, 20, 1, 30, 0) (no tzinfo).

6. No test for the 403-vs-401 boundary

Given the concern in finding #1 above, a test with RuntimeError("HTTP 403 Forbidden") would document whether the intent is to map 403 to ConfigEntryAuthFailed or to None + WARNING. Either choice is defensible but should be explicit.

---

Minor observations

• WholesalePrice docstring embeds a cross-PR maintenance instruction ("do NOT add fields"). This is already captured in DECISIONS.md (D-P7-5 cross-PR contracts). The in-code version will rot; the decisions file is the right place. The docstring can drop to one sentence.

• _DECISIONS.md entries are thorough — the alternatives/consequences structure is particularly useful for audit trail. Well done.

• The CHANGELOG entry is a single dense run-on sentence. Not a bug, but future readers scanning the log will find a bulleted list more scannable.

• conftest.py change is minimal and correct — adds only what this PR requires (ConfigEntryAuthFailed mock).

---

Summary

Blocking	_is_auth_error 403→AuthFailed misclassification (finding #1)
Recommended before 07-02b	float(val) NaN/inf guard (#3), missing multi-point + naive-ts tests (#4, #5)
Non-blocking	_scrub prefix threshold (#2), docstring cleanup

[Artic0din]

Retro-review complete. Findings (if any) tracked in fix/retro-review-batch (#143) or noted in .planning/retro-review-findings.md. Closing synthetic PR + cleaning branches.