Repository navigation
Merge upstream v8.54.3 - #57
Merged
Merged
Conversation
…ort risk The setting's step was 0.1, but the margin that stops Predbat exporting the battery at a loss is the ratio of what exporting earns to what replacing the energy costs - only about 1% on a flat export tariff near the cheapest import rate (11.174p / 11.319p on the 10.54p import, 12p export case in springfall2008#4840). One step below the 1.0 default therefore landed at 0.9, well past the ~0.987 flip point, so the planner force-exported the battery for a real loss of about 0.2p/kWh with no way for the user to nudge the setting gently. 0.02 is not enough either - 0.98 is still below the flip point - so the step becomes 0.01. Every value reachable on the old 0.1 step remains a valid multiple of the new one, so existing settings are unaffected, and all step consumers branch only on whether the step is integral, which is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 403 carrying a CDN block page is already handled for GraphQL queries, which keep their cached JWT rather than discarding it. The token mint sits behind the same CDN but had no such handling, and unlike a query it has no cached result to fall back on: once the JWT expires, every authenticated call needs a new one. The result is a lockout that cannot self-heal. One edge block near token expiry is enough - the mint is refused, so is the retry, and the component re-mints on every poll indefinitely (~50 attempts an hour observed over 26 hours, with no successes). Rates keep downloading over REST throughout, so the integration looks healthy while every dispatch, settings and saving-session query silently returns nothing. Detect the block in async_refresh_token with the existing is_edge_block_body check and back off exponentially instead, 5 minutes doubling to a 1 hour cap, cleared on the next successful mint. A block that lifts is still picked up within the hour, but a block that does not is no longer hammered. The block is also now logged as an edge block rather than as the generic "Unauthenticated request: 403", which reads like a revoked API key and sends anyone diagnosing this down the wrong path. A 403 that is not identifiable as a CDN page keeps the existing refresh-and-retry behaviour, so genuinely revoked credentials still recover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
… off Review catch on the previous commit. The backoff guard sat after the five minute proactive-refresh check, so a token with four minutes of genuine life left was treated as unusable the moment a mint was refused: one transient CDN 403 could cost up to five minutes of queries that would otherwise have succeeded. Before the backoff existed the next poll would simply re-mint and recover, so this was a regression the block itself did not cause. Decode the expiry once and, while backing off, hand back a token that has not actually expired. An expired or undecodable token still returns None. Callers that hit a genuine auth failure clear graphql_token before re-minting, so this can only revive a token we have not been able to renew yet, never a revoked one. Also from review: - Repeat the backoff reason at most every 10 minutes while suppressed. The mint makes no request during the window and so logged nothing, leaving anyone reading a short log window unable to tell a deliberate cooldown from a bad API key - every caller only says "token refresh failed". - Clamp the doubling exponent so a long block cannot evaluate 2 ** block_count without bound. The delay was already capped; this bounds the arithmetic. - Extract the schedule into token_mint_backoff_seconds(). Tests: assert the exact backoff schedule rather than mere monotonicity (a 1, 2, 2, 2... implementation passed the old check), and add two cases the previous ones could not catch - an elapsed deadline reopening the mint with the deadline left set, so a guard that tests the field for presence rather than against the clock fails, and a near-expiry token still being served during a backoff. Both were confirmed to fail against the corresponding mutation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
Second review pass. The previous commit only handled the backoff guard, so the call that actually met the CDN 403 still returned None even when the token it held had not expired - the fallback started one call late, for no reason. The guard would have handed that same token to the very next caller anyway. Return it from the edge-block branch too, re-reading the clock rather than reusing the timestamp sampled before the request went out. Tests: Test 14 asserted the old behaviour, so it was pinning the defect - it now requires the token from the triggering call. Adds an undecodable token during an active backoff (no provable life left, so it must not be served) and a throttle test proving the reason is restated once per interval rather than per call. Both new assertions were confirmed to fail against the corresponding mutation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
is_edge_block_body, the CDN markers and the token-mint backoff schedule are not Octopus-specific: every Kraken-based provider (Octopus, EDF, E.ON) mints its JWT through the same CDN-fronted endpoint and can be blocked the same way. Move them to utils so the Kraken auth mixin can use them too. No behaviour change. EDGE_BLOCK_REFRESH_AFTER stays in octopus.py - it is policy for that component's query path, not a shared helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
The Kraken auth mixin fails in the opposite direction to the Octopus component, from the same trigger. _kraken_token_request returned None for any non-200 without reading the body or logging anything, so a CDN 403 was indistinguishable from a 500 or a rejected credential. check_and_refresh_oauth_token then set oauth_failed, which is a latch: its first line is `if self.oauth_failed: return False`, and nothing clears it outside _init_kraken_auth. So a single transient edge block at refresh time disables the provider for the lifetime of the process - silently, and with no retry - until a restart. That is not a credential problem and should not be treated as one. Read the body on a non-200 and use is_edge_block_body to tell an edge block from a real rejection. On a block: back off on the shared schedule, keep any token we still hold that has not actually expired, and leave oauth_failed alone. Do not retry immediately with primary credentials either - that is a second request into the same block. Genuine failures keep the existing behaviour: retry once with primary credentials, then latch. Other non-200s are now logged rather than swallowed; previously every failure path in _kraken_token_request returned None without a word. Also bind the three new backoff helpers onto the instance in KrakenAPI. When OAuthMixin is the class base only the auth entry points are bound, so a helper called by check_and_refresh_oauth_token has to be bound alongside it or it raises AttributeError at runtime - caught here by an existing test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
…ng tests Second review pass. None of the seven tests added by the previous commit actually ran: run_kraken_auth_mixin_tests() carried a hardcoded list and the new tests were defined below it, so unit_test.py - which calls that runner, not pytest discovery - skipped every one. CDN detection, the non-latching behaviour, suppression, the valid-token fallback and the elapsed-deadline path could all have regressed with the suite green. Both Kraken runners now discover module-level test_* functions instead of listing them, so the next test added cannot go unrun either. Also from review: check_and_refresh_oauth_token compared the retained token against a timestamp sampled BEFORE the request. A token with ten seconds left when a fifteen second edge-blocked request started was reported as usable. Re-read the clock before answering. New coverage for the two gaps the review named: - An edge block while a refresh token is held: it must not burn the refresh token, must not recurse into the same block, and must not latch. - A real KrakenAPI driven through block, suppression and recovery. KrakenAPI binds mixin methods onto the instance one at a time, so every helper reached from a bound method has to be bound alongside it; the direct-mixin tests cannot see that. Confirmed to fail when any one binding is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
…y-value-scaling-step-4840 Reduce metric_battery_value_scaling step to 0.01 and document the export risk
Fixes springfall2008#4851. Octopus publishes Power Up and Power Down through the same savingSessions feed and distinguishes them with `eventType`: TURN_DOWN reduce consumption, rewarded in octopoints WEEKEND_HAPPY_HOUR an earned free import hour, reward 0 TURN_UP increase consumption, rewarded in octopoints Predbat's query never asked for that field, so a booked Weekend Happy Hour arrived indistinguishable from a saving session at 0 p/kWh and was dropped by `saving_rate > 0`. The free hour never reached the plan: the import rate stayed at the standard tariff rate, no charge window was planned, and the only way to use it was to enter it by hand as a manual import rate override. Request `eventType`, carry it through to the joined and available event lists, and route a joined WEEKEND_HAPPY_HOUR into octopus_free_slots at rate 0. Deliberately NOT keyed on the reward value. A zero reward looks like a free hour but a TURN_DOWN reporting 0 - an unknown reward published as 0 rather than null - would then be zeroed on IMPORT, making Predbat grid-charge through a session where the user is meant to be reducing import: full price, believing it free, and the participation lost. eventType is the only sound discriminator. TURN_UP stays on the saving path. It means import more, but it is rewarded in octopoints rather than free, so folding it in here would misprice it. An absent eventType falls through to the existing behaviour rather than guessing, so the BottleCapDave feed and older API responses are unaffected. Tests cover all four: a joined WEEKEND_HAPPY_HOUR becomes a free slot while a rewarded TURN_DOWN in the same joined_events list stays a saving slot, a TURN_DOWN reporting 0 does not become free, TURN_UP does not become free, and a missing eventType does not become free. Reverting to the reward-value test fails two of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
…indow 01e3982 started deriving a meter's off-peak windows from the measurements API's TOU bucket labels, taking whichever label was most common for each half-hour across the last 7 days. That premise holds for Economy 7, where the labels really do describe a fixed schedule, but not for Intelligent Octopus Go. On IOG the off-peak buckets cover the guaranteed overnight window plus whatever ad-hoc dispatch slots Octopus granted that night, and dispatches are decided night by night. A bonus slot that happened to land on a plurality of the sampled nights was therefore promoted to a permanent recurring window applied to every future day. On one user's account (IOG-SMB-VAR) that invented a nightly 22:00-22:30 off-peak period alongside the genuine 23:30-05:30 one, and Predbat planned a battery charge into it believing the rate was 6.9p while the meter billed the day rate of 30.3p. The plurality vote has no consistency threshold, so four nights out of seven was enough. Skip the inference for Intelligent GO tariffs and use the guaranteed window, which is the only fixed part of an IOG day. octopus_night_times still overrides everything for a meter whose real bands the API does not report. Also anchor TOU-derived windows to whichever clock the tariff's off-peak period is actually fixed to, rather than assuming UTC. The labels are UTC instants, so a local wall-clock tariff sampled during BST reports its window shifted by an hour; holding that offset fixed ran it an hour early once the clocks went back - the same class of bug 01e3982 set out to fix, reintroduced for local tariffs. Verified against a reporting user's live account: 6.9p now applies only across 23:30-05:30 local and 22:00-22:30 is back to the 30.3p day rate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The distinguishing symptom is a cheap slot Octopus has never heard of while the dispatch feeds are empty, which points at the day/night rate path rather than phantom dispatches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…s-token-mint-edge-block-backoff fix(octopus): back off when the CDN blocks the token mint
…bels For a local-anchored tariff, the log printed tou_windows (raw UTC minutes) while windows (converted to local wall-clock minutes) was what actually got applied, making the log line misleading during debugging.
…-auth-edge-block-handling fix(kraken): do not treat a CDN block as a permanent auth failure
…s-iog-tou-phantom-window fix(octopus): stop IOG dispatch slots becoming a permanent off-peak window
…s-zero-reward-power-up-free-slot fix(octopus): treat a joined zero-reward session as free electricity
get_log could only narrow by severity, a literal substring and an age in hours. Alternation or anchoring was impossible, and there was no way to ask for a specific window - which is exactly what diagnosing "what happened at 17:06 yesterday" needs. Adds three optional arguments: pattern a case-insensitive Python regex, ANDed with filter and search start lower bound, accepting a date, a time today, or both end upper bound, same formats; a bare date covers the whole day All are additive - filter, search, hours and max_lines keep their existing meaning, so calls that work today behave identically. hours still works and intersects with start, the narrower bound winning. Two supporting changes. compile_filter_argument() gains an optional flags argument (default 0, so the other four regex tools are unaffected) so get_log can match case-insensitively the way its literal search already does. And the scan moves out of _execute_get_log into scan_predbat_log(), which now runs on the executor in full rather than only the file read - a caller-supplied regex walking a 10MB log on the web loop would stall the SSE stream mid-token. Fixes a latent bug the time bounds exposed: a line with no stamp of its own was inheriting the timestamp of the newer entry below it in the file rather than the entry it continues, so a traceback belonged to the wrong error. It was invisible while only the hours filter existed, because both stamps were recent, but a start/end window would detach a traceback from its error. Timestamps are now resolved in a forward pass before the newest-first selection walk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
max_lines bounded how many lines came back but not how big they were, and some Predbat log lines are enormous - one Octopus GraphQL response is a single 20KB line. 500 of those made a 953KB tool result, roughly 238k tokens, which was 97% of one conversation. It overflowed the model's context, so the provider truncated the request from the front, dropped the only user message with it, and answered "no user query found in messages" - a failure that reads as nothing to do with the log tool that caused it. Three guards now, not one: max_lines default drops from 500 to 200 MCP_LOG_MAX_LINE_CHARS any single line over ~1000 chars is cut MCP_LOG_DEFAULT_MAX_BYTES the response as a whole stops at 64KB The per-line cap alone is not enough - 500 lines of 1000 chars is still about 50k tokens - which is why the total budget backstops it. truncated_reason says which guard ended the response so a caller knows whether to narrow the filter or raise the line count. Nothing is lost, only deferred: a cut line carries how many characters were dropped and names its own line number in the marker, and a new line_number argument returns that line in full, with optional context either side. That mirrors search_source pointing read_source at the part worth reading. The filters are deliberately ignored on a read-back, since the caller already knows which line it wants and applying them could only refuse it. Also corrects the comment claiming the line count "keeps a response small enough for an AI context window", which springfall2008#4840 proved it does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Review follow-up. f2c2ce8 moved read_predbat_log_lines() to splitlines() but left scan_predbat_log() on split("\n"), which is worse than either choice applied consistently: the two paths index into the same numbering, so a marker saying "get_log line_number=26793" could read back a different line than the one it was pointing at. Both now go through split_predbat_log(). It uses splitlines(), which also drops the phantom final entry the trailing newline every log ends with left behind - total_lines counted a line that did not exist and the last line number read back as "". splitlines() does also split on \v, \f, \x85 and , which a logged third-party API payload could in principle carry. Measured against a real 57,400-line predbat.log the difference from split("\n") was exactly the one trailing empty, so that risk is theoretical while the phantom line was not. The test now asserts the property that actually matters rather than the splitting mechanism: every line the scan returns must read back as the same line by its own number. That is the regression guard for the inconsistency above, which no existing test would have caught. Also fixes scan_predbat_log's docstring, which still described four return values after truncated_reason made it five, and pins one clock reading per test - _dated_log() and _fat_log() took their own datetime.now() while the tests built bound strings from another, so a run crossing midnight could disagree by a day. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…echo A sent message sometimes never appeared in the transcript until the user switched conversation and back, which rebuilt it from history. The bubble was drawn solely by handleUser(), from the server's 'user' SSE event, so a single missed event lost the message from the view for good. Two ways it goes missing: createAndSend() called doSend() immediately after selectConversation(), but openStream() only runs at the end of that load - so every send on a new conversation raced the stream being opened, and recovery depended entirely on the cursor still being behind the event. _snapshot_and_cursor() takes the message snapshot under the store's lock and the cursor under the agent's, while a turn appends from the component thread. Interleave there and the message is absent from the history with its event already below the cursor: not rendered, not replayed. Rather than chase either race, sendMessage() now draws the bubble itself and handleUser() adopts it instead of drawing a second one. Your message appears the instant you press Enter, and no lost event can cost you it. The echo still renders normally when nothing is pending, which is what a second browser watching the same conversation needs. Supporting changes: renderHistory() clears the pending marker, since it throws away the transcript the marker points into and the rebuilt history already contains the message. doSend() takes the bubble back down if the send fails, so a 409 for a turn already running cannot leave the transcript claiming something was said that was never stored. selectConversation() returns its load promise so createAndSend() can wait for the stream to be open before sending - defence in depth rather than the primary fix. _snapshot_and_cursor()'s docstring claimed no gap existed because no await separates its two statements. That holds for this loop but not across threads, so it now describes the window it actually leaves and why closing it properly is not worth it here. Verified by running the real generated script in a DOM shim: the bubble appears on send, the echo does not duplicate it, an echo with nothing pending still appends, renderHistory clears the marker, and an echo after that wipe renders rather than being wrongly adopted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ceiling
/api/show returns the context length baked into the model - 262144 for
Qwen3.8 27B - and the picker reported that. The server caps every model at
OLLAMA_CONTEXT_LENGTH, which on macOS is an Ollama app setting rather than an
environment variable, so the same model is loaded with 131072 behind a 128k
cap. Two Qwen variants advertised 256k while the server would only ever give
128k.
That is worse than a wrong number in a dropdown. The Chat tab's context
counter measures fullness against it, so a doubled limit reads half-full at
the point the window is genuinely full - no warning before a turn overflows,
which is the failure GH#4840 ran into from the other direction.
/api/ps carries context_length per loaded model, which is the effective
value. It is read once per enrichment pass and used in preference to the
ceiling. A model the server has never loaded is not listed there and keeps
its own figure: that is the only answer available and still better than
reporting the ceiling for everything.
Only the raw /api/tags response is cached; _catalogue_to_models() and this
enrichment run on every list_models() call, so the reported figure tracks
what is loaded rather than being frozen for a day.
A server too old to serve /api/ps, or one with nothing loaded, returns an
empty map and every model keeps its ceiling - the behaviour before this
change, so nothing degrades.
Verified against a live Ollama 0.33.2: the helper returns
{'qwen3.8:27b': 131072} where /api/show reports 262144.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ollama Cloud refuses /api/ps. Verified against ollama.com: a key good enough for /v1/models and /api/tags (both 200) still gets 401 there, because "loaded" is a local-server idea and there is nothing to report for a shared fleet. Sending the key does not help. The failure was already ignored - a non-200 leaves every model on its own ceiling - but nothing tested that, and a cloud provider spent a guaranteed 150ms round trip on every enrichment pass to learn it. Only a locally loaded model can have been capped, so a catalogue whose models are all remote now skips the call entirely. Two tests, both mutation-checked. The first pins the fallback: a 401 and a transport error each leave the ceiling in place. Its 401 body deliberately carries a plausible models array, because the first version of this test passed even with the status check deleted - a refusal with an empty body is indistinguishable from a refusal that is handled, and the downstream shape check was quietly doing the work. With a poisoned body the mutation is caught (the model came back as 4096). The second pins the skip: a cloud-only catalogue must make no /api/ps call at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up. The byte budget broke out of the scan, so matched_lines tallied only the lines that fitted. The description reports "the most recent X of Y matching lines", and a Y that stops at the cap understates exactly what that sentence exists to convey - measured at 71 against a true 400. max_lines already continued past its cap and kept counting, so the two truncation paths disagreed. Both now walk to the end of the window and append nothing further. Also fixes the accounting either side of the check: `used` was incremented before testing the budget, so the line that pushed the response over was billed for despite never being appended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up, keeping the behaviour and documenting why. The suggestion was to use `is not None` so a valid zero is not treated as missing. No model has a zero-token window, so a 0 from either endpoint is a placeholder or a bug rather than a value worth preserving, and taking it would replace a usable figure with one that tells the user nothing. Applying the suggestion verbatim makes a model with a 262144 ceiling report a context_length of 0, which the new test catches. The whole chain already agrees that falsy means "no answer": _ollama_loaded_context() keeps only truthy values, contextLengthForModel() returns `context_length || null`, and renderContextUsage() shows the token count alone when the limit is falsy - its comment says so explicitly. Nothing changes except a characterisation test pinning the choice, and comments at both sites so the intent is not read as accidental truthiness again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…og-pattern-and-date-range Add regex and start/end time bounds to the get_log tool
Co-Authored-By: Claude Code <noreply@anthropic.com>
Review nit: the new backoff/token-request logs read "Warn: Kraken token ..." while the rest of the component uses "Warn: Kraken: ...". The odd one out breaks grep and log-parsing on the usual prefix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
Review follow-up (@gcoan). A Weekend Happy Hour cannot be joined through the API - Octopus either allocates one or the user books it on the website - so listing it in available_events only produces join attempts the API rejects (springfall2008#4593/springfall2008#4595) and fills the join selector with options that cannot be selected. BottleCapDave's integration stopped passing these through in v19.0.1 for the same reason; match that now that eventType makes them identifiable. Ordering matters and is easy to get wrong: the reward/code/type maps are built from this same events list, and a joined Happy Hour looks its event type up there. Skipping before those maps are filled leaves the joined event with no type and the injected default reward, so the planner prices a free hour as an 80p/kWh saving session. The skip therefore sits after the maps are populated, and the test fails if it is moved above them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
… reporting them /pr-cleanup previously only gathered review feedback and CI status - a PR that was merely stale/conflicting against main (no comments, no CI) fell through as "nothing to address", even when GitHub reported it CONFLICTING (observed on PR springfall2008#4036). Adds a new step 2 that merges origin/main into the branch and resolves any conflicts by hand, aborting and asking for a manual rebase instead of guessing when the conflict is too large to resolve safely. Grants the BOT_CLEANUP flow the git-merge permission it needs to act on this (no other flow checks out an existing branch that can be behind). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Issue springfall2008#4881: the first triage attempt against glm-5.3-flash:cloud completed its real work (comment posted, BOT_TRIAGED applied), then kept running until Claude Code's Anthropic-priced cost estimate crossed the $10 cap and aborted with a non-zero exit - which made the daemon retry an already-finished issue. claude_budget_args() omits --max-budget-usd entirely for whichever flows are running against an Ollama model; --max-turns remains the circuit-breaker there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ditor hangs - Narrow the BOT_CLEANUP git-merge permission from an unscoped "git merge*" to enumerated origin/main-scoped spellings (bare, --no-edit, --abort), matching pr-cleanup/SKILL.md's own "only ever merge origin/main" guardrail and this file's established pattern for the flag-before-target literal- prefix-matching footgun. - Switch the skill's example commands to --no-edit, both to stay covered by the narrowed allowlist and to avoid hanging on an interactive editor prompt with no one there to dismiss it. - Fix SKILL.md wording that implied a fast-forward merge always creates a merge commit, and a test docstring's awkward "reach without a use" phrasing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eanup-resolve-merge-conflicts Triage bot: have BOT_CLEANUP resolve merge conflicts against main
…-kwp-wp-warning-4858
…-bot-comment-disclosure Triage bot: disclose bot authorship on every comment it posts
…, tighten the tests fetch.py: - The subtract now respects load_baseline. The floor is applied in the fill loop and the modulation walked straight back through it - only reachable at all since the load arrays started being modulated. The new non-flat test catches it: 144 of 290 steps fell below a binding floor before this. - Key the phase off the minutes_now parameter rather than self.minutes_now. Every other minute in the function comes from the parameter and output.py already calls it with 0; this also makes the result a pure function of its inputs, which the replay and test paths need. - Express the grid in step units instead of a hardcoded 5, so a future step=10 caller gets alternating parity rather than a constant one and a .get that always misses. - Don't borrow on the final step: the fill loop runs a plan interval past forecast_minutes, so the last on-step could borrow from a bucket this loop never reaches to repay, leaking it into the total. - Reword the "keeps the same total" comment, which was not true of the code below it. Repaying exactly needs the outstanding debt tracked separately from the next borrow, which is a rewrite of the model rather than a fix here. Tests: - New non-flat fixture. On a flat series the model is net-zero by construction, so the original fixture could not see under-repayment, a breached baseline floor or a negative step. This asserts the total never rises, no step goes negative, and the floor holds. - CLOUD_FACTOR_PV10 promoted to const.py and imported by plan.py, test_single_debug and test_random_scenarios, which each spelled the 0.2 increment separately. - Dropped the cloud_factor=0 assertion: it takes the identical guarded path as None, so it could never fail independently. - test_pv90 gains a companion asserting the pv90 < nominal < pv10 ordering holds with divergence active, restoring coverage of the default-on modulated path the clamp test has to disable. Its saved-dict now uses a getattr default so changed seeding reports rather than raising. Baselines: debug_cases goldens re-verified end-to-end against the fixed tree, each case run individually - both unchanged. random_results regenerated (runtime_s preserved). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GivEnergy answers a rejected setting write with HTTP 200 and puts the failure
in the body, e.g. {"value": -7, "success": false, "message": "Inverter Locked"}.
Predbat took the 200 at face value: it refreshed the component health timestamp
and recorded a successful API call for a write that never reached the inverter,
discarded both the code and the message, and then retried codes GivEnergy
documents as non-retryable for the full budget (~55s per failing setting).
async_get_inverter_data now classifies a success=false body: it counts the
failure, records it under a specific metrics reason (-7 becomes inverter_locked),
leaves last_success_timestamp untouched so the component health check sees the
outage, and logs GivEnergy's own code and message. Terminal codes (-3, -4, -7)
raise GECloudTerminalError so the caller aborts on the first response; retryable
codes (-1, -2, -5, -6) return None and keep the existing retry behaviour.
The read, write, EVC command and generic retry paths all handle the exception,
and the read and write paths also treat a terminal code as final when it arrives
in a body that did not set success=false.
Fixes springfall2008#4896
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s=false Keying classify_ge_failure on the success flag alone missed the other shape the setting endpoints use, where the negative code arrives in "value" with no success=false alongside it. Those bodies still refreshed last_success_timestamp and were recorded as successful API calls - the exact defect this PR set out to fix, in the response shape it did not cover. A known code is now a failure in its own right, but only on the setting read and write endpoints, where "value" carries a result code; anywhere else a negative "value" could be a genuine reading, so an explicit success=false is still required there. A retryable failure now hands the body back to the caller rather than returning None, so the read and write paths keep classifying the code themselves and keep their request-spreading jitter for the timeout codes - retry behaviour is unchanged from before the PR, only the metrics and health timestamp move. That in turn makes the terminal-code branches in those two paths unreachable, since async_get_inverter_data now always raises for them, so they are removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Updated automation steps for mid-window time updates and added instructions for configuring the soc_kw sensor and apps.yaml.
PR springfall2008#4500 added self.args_from_apps_yaml, a deep copy of self.args taken at the top of initialize() before Predbat's own defaulting runs, so set_arg_auto() can tell "the user configured this in apps.yaml" apart from "Predbat defaulted it" and warn when auto-discovery overrides an explicit setting. Both dump paths that walk self.__dict__ special-cased only the field literally named "args" for masking, and is_debug_excluded_key() tests the attribute *name* (which contains none of _key/password/secret/token). So this second, unredacted copy of apps.yaml went out in the clear alongside the properly-masked one: - create_debug_yaml() - the predbat_debug.yaml users attach to bug reports, and every rolling debug_history snapshot - the get_state agent/MCP tool - handed to whichever model asked Real api keys, tokens and passwords were included in all of them. Fixed at the source: the snapshot is now built with mask_secret_args() rather than copy.deepcopy(), so no secret ever enters the structure regardless of how it is later dumped. This costs nothing functionally - set_arg_auto() is only ever called with auto-discovery targets (battery_scaling, soc_max, car_charging_energy, sensor entity ids) across alphaess, sunsynk, solis, myenergi, ohme and gecloud, never a credential-shaped key, so the values it compares and logs are unaffected. Both dump sites also mask the field explicitly. That is belt-and-braces given the source fix, but keeps the two paths consistent about which fields are credential-bearing so a future field added next to it is not silently exempt. Adds test_args_from_apps_yaml_snapshot_is_redacted, which plants an unmasked snapshot and asserts neither create_debug_yaml() nor get_state emits it. Verified by mutation: reverting either masking tuple fails it on all ten leak assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… them SECRET_KEY_SUBSTRINGS can only catch what a key *name* reveals (_key, password, secret, token), so credentials named after what they identify were served in the clear: account numbers, meter point numbers, site/system/plant ids, login identifiers, and the id half of an id/secret pair. Adds a "secret": True flag to the component registry's arg definitions, so a credential is declared next to the arg that supplies it rather than inferred from its spelling. components.secret_config_names() reads the live COMPONENT_LIST (picking up the conditionally registered gateway component) and feeds utils.is_secret_key(). 23 credentials that previously went out unredacted are now covered: octopus_api_account, kraken_account_id, kraken_export_account_id, kraken_mpan, kraken_export_mpan, kraken_email, myenergi_hub_serial, ohme_login, deye_username, deye_app_id, deye_company_id, sunsynk_username, alphaess_app_id, enphase_username, enphase_site_id, axle_site_id, axle_partner_username, sigenergy_system_id, sigenergy_client_pem, teslemetry_site_id, solax_client_id, solax_plant_id, gateway_device_id myenergi_hub_serial is included because it is the digest auth username, not just a device id (myenergi.py:393). The 36 args the substrings already caught are flagged too, so the registry is the single source of truth and a test asserts the two agree. Redaction and the '!secret' advice are deliberately separated. is_secret_key() takes registry=True by default - the strict answer to "must this be redacted?" - so a new caller fails safe. find_unmasked_secret_paths(), which drives the "stored in plain text, consider !secret" banner, passes registry=False: that advice is about values which grant access, and an inline octopus_api_account is the documented normal setup, so warning every user about one would be noise rather than advice. Note this scope also binds find_redacted_secret_overwrite(), which must agree with the redactor or saving the apps.yaml editor would write 'xxx' over a real account number. Device serial numbers (solis_inverter_sn, ge_cloud_serial, solax_plant_sn and friends) are deliberately left readable: they address hardware rather than authenticate it, and they are what makes an integration bug report diagnosable - the same trade-off SECRET_KEY_EXEMPT_SUFFIXES already makes for token expiry times. utils cannot import components at module scope (every component module imports utils), so the registry is imported on first use. An empty or failed lookup is not cached, so a redaction running while components is still importing resolves properly on the next call instead of silently losing these names for the life of the process. Adds test_registry_secret_flags_drive_redaction, asserting every flagged arg redacts, that the newly covered names redact end to end through mask_secret_args and get_apps, that serials and other non-credentials stay readable, and that the !secret banner keeps its narrower scope. Mutation-verified: removing the registry consultation fails 50 assertions, dropping one "secret": True flag fails 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The /debug_apps_live masking test only injected octopus_api_key, which the key-name substrings catch on their own, so it could not tell whether the component registry's "secret" flag reaches this route. Adds octopus_api_account alongside it - an account number reads like ordinary config, so before the flag existed this download served it in the clear. Asserts it is redacted on both a bare request and masked=1, and still returned under the deliberate masked=0 opt-out. Mutation-verified: removing the registry consultation from is_secret_key() fails both masked assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d-inverter-locked-4896 GE Cloud: surface write rejections instead of counting them as successes
The "apps.yaml (file)" link served apps.yaml verbatim, with no redaction and no warning, while the "apps.yaml (live)" link beside it defaulted to masked and offered the unmasked copy behind a confirm dialog. Someone grabbing a file to attach to a bug report had a fair chance of clicking the one that hands over every credential in their config. /debug_apps now takes ?masked= and defaults to masked, matching /debug_apps_live. The default matters more than the dialog: the config-errors page carries its own plain './debug_apps' link (web.py:3673), so defaulting to masked covers that one too rather than only fixing the link the dialog is attached to. downloadFileApps() gives the Download row's file link the same OK/Cancel choice as the live one. Redaction is applied to the file text, not the parsed args - mask_secret_yaml_text() round-trips through ruamel so the download still reads like the user's own apps.yaml, comments, ordering and quoting intact, with only credential values replaced. A '!secret name' reference is left exactly as written: it holds no credential, only the name of one, and which secret a key resolves to is what makes a misconfigured integration diagnosable. Unparseable YAML raises, and the route fails closed with an explanatory comment block rather than falling back to the raw file - serving unredacted text because the parse failed is the leak this exists to prevent. The editor is deliberately left alone, on both halves: /apps_editor shows the real file (a credential you cannot see is one you cannot edit) and writes back exactly what was submitted. Redaction applies only to downloads, which produce a file for someone else; the editor produces the file Predbat runs on, so masking it would save 'xxx' over real credentials. test_web_if now pins that invariant - mutation-verified by wrapping the editor's write in mask_secret_yaml_text(), which fails both new assertions. Also asserts unknown keys still redact on their name alone (supabase_key, whatever_password and friends): the registry adds to the substring heuristic rather than replacing it, and collapsing is_secret_key() to registry-only fails those assertions plus the existing nested-credential test. The /debug_apps test writes its credentials into the file on disk rather than into my_predbat.args. The shipped test apps.yaml carries no credential, and an earlier POST /apps_editor in the same test has already ruamel-round-tripped the file, so comparing raw against redacted text passed whether or not anything was redacted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…modulation-precedence fix(fetch): make the cloud/load divergence model actually modulate
test_training_does_not_copy_the_dataset_per_epoch failed on every macOS machine, including on clean main, reporting ~280MB of growth against a 143MB budget and concluding "the matrix is being materialised". The matrix is not being materialised. What the guard actually measured was ru_maxrss, a high-water mark of pages *ever touched* rather than memory held at any instant. Training frees each batch buffer immediately - measured here it retains 0.0MB of live numpy arrays after a pass and peaks at 67MB of Python-tracked memory - but the mark accumulates every page those thousands of short-lived buffers ever dirtied. The mark is also page-granular, so a 16KB-page host (Apple Silicon) inflates it several-fold against a 4KB-page Linux one. That made the budget, which was calibrated against a matrix size in bytes, compare two quantities that are not commensurable. It went unnoticed because this is a slow test and CI runs --quick, so it has never been exercised upstream - neither green nor red. Assert the invariant directly instead: WindowedFeatures.__array__ is the only path that builds every row at once, so count its calls, and separately check that no single allocation approaches the notional matrix. Both are properties of the code rather than of the host. Verified the guard still fails when it should by forcing a materialisation: it reports materialised=1 and a 65.9MB allocation, tripping both checks. In normal operation the largest single allocation is 5.9MB against a 71.6MB matrix. Peak RSS is still reported as a diagnostic, just not asserted on, and benchmark() still returns it for manual profiling. The module docstring previously suggested MallocLargeCache=0 to get a Linux-comparable figure on macOS; measured, it changes nothing (279.8MB vs 282.9MB), because allocator caching is not what the mark records. Replaced with what the number actually means. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rom-apps-yaml-secret-leak fix(security): stop apps.yaml downloads and debug dumps leaking credentials
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…mid-window-automation docs: add Hanchu iESS mid-window charge/discharge time update automation
…s-power-up-review-follow-ups fix(octopus): review follow-ups for springfall2008#4849, springfall2008#4850 and springfall2008#4852
…ining-perf-rss-guard fix(test): assert the ML training invariant instead of peak RSS
Conflicts resolved: - predbat.py: keep the fork version scheme, bump to v712.30 - annual.py: keep both HEADLESS_APPS_YAML_NAME and upstream's new config_warnings helpers - web_annual.py: union the annual imports (RESULTS_SCHEMA_VERSION plus upstream's SOLAR_KWP_SOFT_LIMIT/config_warnings) - tests/test_web_mcp.py: the fork's OAuth suite owns this filename and upstream's tool tests live in test_web_mcp_tools.py, so upstream's new get_log byte-budget and line-number tests were applied there instead Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016mak6yDBWtLnwGGN92NG5w
dandwhelan
marked this pull request as ready for review
September 4, 2026 09:02
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
6 participants
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.
Merges upstream
springfall2008/batpredv8.54.3 (66 commits since the v8.54.1 base) into the fork.Conflicts and how they were resolved
apps/predbat/predbat.py— kept the fork's version scheme and bumpedTHIS_VERSIONtov712.30.apps/predbat/annual.py— kept both sides: the fork'sHEADLESS_APPS_YAML_NAMEconstant and upstream's new_solar_entries/_numeric_or_none/config_warningshelpers.apps/predbat/web_annual.py— unioned theannualimport line: the fork'sRESULTS_SCHEMA_VERSIONplus upstream'sSOLAR_KWP_SOFT_LIMITandconfig_warnings.apps/predbat/tests/test_web_mcp.py— the fork rewrote this filename to hold the MCP OAuth/JWT/PKCE suite and moved upstream's tool tests totests/test_web_mcp_tools.py. Kept the fork's file, and applied upstream's 404 new lines (get_log byte budget, line-number read-back, trailing-newline counting) totest_web_mcp_tools.pywhere those tests actually live.Fork features verified intact
config.py,fetch.py,prediction.py,plan.py, plus theprediction_kernel.pyfallback gate andtests/test_fit.py/dash_entitiesdashboard inweb.py/web_helper.pyDEFAULT_PREDBAT_REPOSITORY = "dandwhelan/batpred"Upstream touched no kernel source (
prediction_kernel.cpp,prediction_kernel.py,prediction.pyhot loop) between v8.54.1 and v8.54.3, so no ABI bump or.sorebuild was needed.Testing
unit_test.py --quick— all pass (5 slow tests skipped, 375s)pre-commit run --all-files— all hooks pass🤖 Generated with Claude Code
https://claude.ai/code/session_016mak6yDBWtLnwGGN92NG5w
Generated by Claude Code