chore: retro-review PR #109 — fix(codex): P0-1 token-out-of-URL + P0-2 reset-all-providers + P1-6 cancel-on-un#139
Closed
Artic0din wants to merge 3 commits into
Closed
Conversation
…lover Codex full-repo review (2026-05-23, P0-2) flagged that the daily rollover persists yesterday's history but only resets the Amber provider's accumulators (and only inside the monthly-reset branch lower down). DWT, CdrPlan, FlowPower, and LocalVolts providers carry ``today_cost`` across days indefinitely — corrupting Energy Dashboard cost sensors and external statistics. Add a `for provider in self._providers.values(): provider.reset_daily()` loop after history capture, wrapped in try/except so a single buggy provider can't take out the whole day's reset. Logs the offending provider's id on failure for triage. 3 source-level regression tests pin the contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tasks Codex full-repo review (2026-05-23, P1-6) flagged that PR #107 switched the initial ranking job + backfill scheduler to ``hass.async_create_background_task`` to fix the bootstrap-blocking bug, but didn't retain the task handles. On reload/unload the tasks survive against an unloaded coordinator — visible as pytest "coroutine was never awaited" warnings AND as real data races in production when a reload happens mid-ranking. Capture both task handles and register ``entry.async_on_unload( task.cancel)`` for each. HA now cancels both cleanly on reload/unload. 3 regression tests pin the contract (in the same file as P0-2's tests): - ranking task handle captured + cancel-on-unload registered - backfill task handle captured + cancel-on-unload registered - bare ``hass.async_create_task(...)`` never re-introduced for these call sites (regression guard against accidentally reverting PR #107 while patching task lifecycle) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex full-repo review (2026-05-23, P0-1) flagged that `setup_panel_iframe` appended the HA long-lived access token to the iframe URL as `&token=<jwt>`. Tokens in URLs leak via: - browser history - HTTP Referer headers - screenshots and screen-share - panel-config dumps (HA backups, frontend storage) - log files (despite the existing `&token=<REDACTED>` log redaction — the redaction only covered our log line, not the other paths) The iframe page (`dashboard.html`) already has a four-method auth fallback chain documented inside the file: 1) URL ?token= (the leaky path) 2) parent.hassConnection (panel-iframe parent frame) 3) localStorage.hassTokens (HA frontend auth cache) 4) parent.localStorage.hassTokens Methods 2/3/4 work in HA's same-origin iframe context — the iframe loads from `/local/...` which shares an origin with the HA frontend. Removing method 1 from the Python side leaves the page authenticating cleanly via the existing fallback paths. dashboard.html itself is unchanged — keeping method-1 in the JS as dead code is harmless and protects users running stale dashboards during the upgrade window. Future cleanup PR can strip it. The new Lit `setup_panel_custom_v2` panel (Phase 10 PR-13) is the long-term replacement — it never used URL tokens. The iframe panel stays available during the migration window per its existing intent. 2 regression tests pin the contract: - no `&token=` concatenation inside `setup_panel_iframe` - no `entry.data["ha_token"]` access patterns inside the function 1078 passing (+2), ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@claude review |
|
Claude encountered an error after 14m 3s —— View job Code Review — PR #139 (retro-review of #109)
Tasks
|
|
Claude encountered an error after 11m 2s —— View job PR #139 — Retro-review of PR #109
|
Owner
Author
|
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. |
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.
Synthetic retro-review of merged PR #109. Diff = original
535c1d2..68da94b(same as original PR). Do NOT merge; close after @claude review posts.