TARIFF-02 review: close the three other doors to the markup, and stop overclaiming

Review of PR #16 wrote a double-counting plugin four ways. PluginOptions
blocked one. The one that decided it touched nothing but
PRICE_OPTION_DEFAULTS - a public dict in the module every plugin must
already import for register() - so "the number is not in scope" was
simply false, and an absolute guarantee a reviewer trusts is worse than
a stated limit, because they stop checking.

register() now refuses, AT IMPORT, any plugin whose own code mentions a
framework-owned name: FRAMEWORK_KEYS plus PRICE_OPTION_DEFAULTS,
price_option and _o. code_leaks() parses the class and drops every
docstring node before searching, because every plugin is REQUIRED to
name its sourced components in prose - engie_dynamic says "markup" in
its docstring and a raw text scan false-positives on the documentation
the same file mandates. A check that fires on required documentation is
a check somebody deletes. test_price.py asserts that a raw scan really
would have false-positived, so the strip is proven load-bearing rather
than asserted.

It fails OPEN if inspect.getsource() cannot find the .py, which the
add-on image always can. Refusing to start a battery controller because
a static analysis could not run would trade a real outage for a
hypothetical accounting error - so instead the checks assert source was
found for all five shipped plugins, and a build that lost it is a red
check rather than a quietly disarmed guard.

All three reachable attacks are written out as plugins in the suite and
each must be refused at registration. The cost is measured, not
asserted: honest wholesale 0.188, every attack 0.219 - one markup high,
every hour, invisible in every log this add-on writes.

The guarantee is now stated at its true size everywhere it appears: the
framework adds the complement exactly once, GIVEN A CORRECT sources
DECLARATION. A plugin that declares {ENERGY} while its feed is all-in
still doubles while touching no framework name, and no code can catch
that - the declaration is an unverifiable claim about somebody else's
tariff. Measured in the suite (0.276) so the limit is a number rather
than a caveat. The docstring/declaration check verifies that a plugin's
two claims AGREE, not that either is true; that now says so.

The three surviving mutations:

M3 - _NoData's reason taking a default. This is the identical survivor
left open on RIG-01's _Unread.cause: the shape was borrowed from
sim/riglock.py including its blind spot, which is what borrowing a shape
does if nobody re-runs the mutations against the copy. Closed by
SIGNATURE - no constructor or handler of an outcome type may carry a
default - so the next one is red without anyone writing a new case.

M5 - the ENERGY-required guard was untested. Its consequence is that
adds becomes {ENERGY, TAXES} and the emitted price silently omits the
energy it is pricing.

M7 - deleting the TLS refusal went green AND the check named for it
still passed, because the old parser stripped a fixed len("https://")
and mangled http://api.energy-charts.info/p into the host
pi.energy-charts.info, which then failed the ALLOW-LIST. An assertion
that does not test what it names, inside a security check. Fixed on both
sides: the scheme is parsed as a scheme, so an http URL now resolves to
its real and declared host and only the TLS rule can refuse it; and the
check asserts the REASON. Fixing it surfaced M7b - the no-scheme guard
was survivable the same way - found by mutating my own fix, and closed
the same way.

Also carried forward: rule 7's refusal is correct forever but its stated
reason ("the provider decides nothing") expires the moment TARIFF-03 or
OPT-04 lands. test_price.py section 12 now greps app/ by AST for a
load_prices consumer and goes red when one appears, telling whoever
lands it to update the message in the same commit. The first draft of
that tripwire used a substring and matched the comment in validate.py
explaining the tripwire - the same false-positive class as the plugin
docstrings, hit twice in one ticket, and fixed the same way.

Documented rather than coded around: vars(read).get("points", []) and
bool(read) both still get past the absence type, and neither is worth
code - defining __bool__ would turn a line that reads like a guard into
one that IS a guard by accident, which is how .get(k, default) got here
in the first place.

Stated plainly in DOCS/CHANGELOG/PR: every fixture is synthetic, so the
five feed URLs and JSON shapes are verified against a belief about what
those suppliers publish rather than against the feeds. A wrong shape
degrades to no-price-data, not to a wrong price.

Review's own DST verification recorded: 149,040 hourly instants,
2020-2036, zero mismatches against zoneinfo. The residual is political,
not arithmetic, and the docstring now says so.

24 mutations driven through the suite, 0 survivors. Whole add-on suite
green. No deleted lines outside this ticket's own files; main.py still
untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC

goodwe_controller/app/price.py

@@ -26,7 +26,8 @@ be taken on a provisional price).
 THE THREE THINGS THIS MODULE IS SHAPED BY
 =========================================
 
-1. DOUBLE-COUNTING MARKUP IS MADE UNREACHABLE, NOT DISCOURAGED.
+1. DOUBLE-COUNTING MARKUP IS REFUSED, NOT DISCOURAGED - GIVEN A CORRECT
+   DECLARATION.
    `generic_dayahead` reads a raw EPEX figure and the user's markup and taxes
    have to be added to it. Engie and TotalEnergies publish a figure that already
    HAS the markup in it. Add the configured markup to that one too and every
@@ -41,11 +42,25 @@ THE THREE THINGS THIS MODULE IS SHAPED BY
      * `assemble()` - the framework, never a plugin - adds exactly the
        components the plugin declared it did NOT source, from
        `price_markup_eur_kwh` / `price_taxes_eur_kwh`.
-     * ⚠️ A PLUGIN IS NEVER HANDED THOSE TWO NUMBERS. `fetch()` receives a
-       `PluginOptions`, which is the options dict with the framework-owned keys
-       REMOVED - reading `price_markup_eur_kwh` from inside a plugin raises
-       `FrameworkOption` rather than returning a number. That is what turns
-       "the plugin must not add markup" from a rule into an inability.
+     * ⚠️ A PLUGIN CANNOT REACH THOSE TWO NUMBERS BY ANY ROUTE, and the word
+       "any" was earned the hard way. `PluginOptions` raises `FrameworkOption`
+       on them, which closes the documented path - but review of PR #16 wrote a
+       double-counting plugin THREE more ways, one of them touching nothing but
+       `PRICE_OPTION_DEFAULTS`, a public dict in the module every plugin must
+       already import for `PriceProvider` and `register`. One closed door in a
+       room with three is not a guarantee. So `register()` now also REFUSES ANY
+       PLUGIN WHOSE OWN CODE MENTIONS A FRAMEWORK KEY - see `code_leaks()`.
+
+   ⚠️ THE PRECISE SHAPE OF THE GUARANTEE, because an overclaim here is worse
+   than no claim: **the framework adds the complement exactly once, GIVEN A
+   CORRECT `sources` DECLARATION.** A plugin that declares `{ENERGY}` while its
+   feed is really all-in still comes out one markup high, and NO CODE CAN CATCH
+   THAT - the declaration is the plugin's unverifiable claim about somebody
+   else's tariff. What is mechanised is that a plugin cannot obtain the numbers
+   and cannot apply them; what remains human is whether it described its feed
+   truthfully. The docstring/declaration check in test_price.py verifies that
+   the plugin's two claims AGREE, not that either is true, and it is worth
+   knowing which of those two things you are reading.
 
 2. THE PRODUCT TYPE IS DECLARED, NOT INFERRED FROM THE PLUGIN'S NAME.
    `eneco_eddy` is a real-time / imbalance product (§9.2) and §8.3 says those
@@ -104,10 +119,18 @@ TIMEZONE, AND WHY THERE IS NO `zoneinfo` HERE
     2026 transitions and counts the points rather than asserting 24.
 
 The EU rule is fixed in law and needs no database: CEST from 01:00 UTC on the
-last Sunday of March until 01:00 UTC on the last Sunday of October.
+last Sunday of March until 01:00 UTC on the last Sunday of October. Review of
+PR #16 cross-checked this against `zoneinfo`'s Europe/Brussels over 149,040
+hourly instants, 2020-2036 inclusive, and found ZERO mismatches - so the
+residual risk here is not arithmetic, it is political: if the EU ever abolishes
+the seasonal change, `brussels_offset()` needs editing. That is the same
+exposure as a machine whose `tzdata` has gone stale, and it is one edit in one
+function rather than a package upgrade.
 """
 
+import ast
 import csv
+import inspect
 import io
 import json
 import logging
@@ -116,6 +139,7 @@ from abc import ABC, abstractmethod
 from dataclasses import dataclass
 from datetime import datetime, timedelta, timezone
 from enum import Enum
+from textwrap import dedent
 
 _LOG = logging.getLogger("goodwe.price")
 
@@ -425,6 +449,20 @@ class _NoData(PriceRead):
     `for p in read.points`, `read.get("points", [])` and `len(read.points)` all
     raise `Absence` here rather than quietly reporting that the cheapest hour of
     the day costs nothing.
+
+    ⚠️ TWO WAYS PAST IT SURVIVE, BOTH FOUND IN REVIEW OF PR #16, AND NEITHER IS
+    WORTH CODE:
+
+      * `vars(read).get("points", [])` returns `[]`. `vars()` bypasses
+        `__getattr__` entirely; nothing short of `__slots__` plus a metaclass
+        would stop it, and anyone writing `vars()` on a variant type has left
+        the road on purpose.
+      * `bool(read)` is `True` for BOTH variants, so `if read:` looks like a
+        check and is not one. Defining `__bool__` to be False here would make
+        `if read:` "work" - and that is exactly the wrong fix: it would turn a
+        line that reads like a guard into one that IS a guard by accident,
+        which is how `.get(k, default)` got into this codebase in the first
+        place. Use `.match(...)`. There is no falsy variant on purpose.
     """
 
     kind = "no_data"
@@ -492,8 +530,13 @@ class PluginOptions:
     """The options dict, minus the keys only the framework may read.
 
     Reading `price_markup_eur_kwh` here raises `FrameworkOption` rather than
-    returning 0.03. That is the difference between "the plugin must not add the
-    markup" being a convention and it being a thing a plugin cannot do.
+    returning 0.03.
+
+    ⚠️ THIS IS ONE OF THE TWO DOORS, NOT THE ONLY ONE. `_o` is reachable, and
+    `price_option()` and `PRICE_OPTION_DEFAULTS` are public names in this
+    module. This class alone is a convention; it is `register()`'s
+    `code_leaks()` scan that makes it an inability. Do not weaken either half
+    on the strength of the other.
     """
 
     def __init__(self, opts: dict):
@@ -581,6 +624,62 @@ class PriceProvider(ABC):
         """
 
 
+# ⚠️ THE NAMES A PLUGIN'S OWN CODE MAY NOT MENTION. FRAMEWORK_KEYS is the
+# obvious half; the other three are the routes review of PR #16 actually used to
+# write a double-counting plugin after `PluginOptions` was in place:
+#
+#   `_o`                     - PluginOptions' private dict, reachable as
+#                              `opts._o["price_markup_eur_kwh"]`.
+#   `price_option`           - a PUBLIC framework helper that takes the raw
+#                              options dict and answers with the markup.
+#   `PRICE_OPTION_DEFAULTS`  - a PUBLIC module constant. This is the one that
+#                              decided the review: no private access, no options
+#                              dict, no helper - just a dict in the module every
+#                              plugin already imports for `register`.
+#
+# Any of them turns +0.088 into +0.119 on a wholesale feed with nothing
+# anywhere reporting it.
+BANNED_IN_PLUGIN_CODE = tuple(sorted(FRAMEWORK_KEYS)) + (
+    "PRICE_OPTION_DEFAULTS", "price_option", "_o")
+
+
+def code_leaks(cls) -> list:
+    """Framework-owned names appearing in a plugin's own CODE. Empty is clean.
+
+    ⚠️ DOCSTRINGS ARE STRIPPED BEFORE THE SEARCH, AND THAT IS NOT COSMETIC.
+    Every shipped plugin's docstring is REQUIRED to name the components it
+    sources, so `engie_dynamic` says "markup" in prose and would false-positive
+    against `price_markup_eur_kwh` on a raw text scan. The check that fires on
+    the documentation it also mandates is the check that gets deleted. So the
+    class is parsed, every docstring node is dropped, and the search runs on
+    `ast.unparse` of what is left - i.e. on code, which is the only thing that
+    can actually add a number twice.
+
+    ⚠️ FAIL-OPEN ON MISSING SOURCE, DELIBERATELY. `inspect.getsource` needs the
+    .py file, which the add-on image has (Dockerfile COPYs `app/`) but a frozen
+    or zipapp build would not. Refusing to start a battery controller because a
+    STATIC ANALYSIS could not run would trade a real outage for a hypothetical
+    accounting error, so an unreadable source returns `[]` and logs. CI proves
+    the scan really runs: test_price.py asserts source was found for all five
+    shipped plugins, so a build that silently lost it is a red check, not a
+    quietly disarmed guard.
+    """
+    try:
+        src = dedent(inspect.getsource(cls))
+        tree = ast.parse(src)
+    except (OSError, TypeError, IndentationError, SyntaxError) as e:
+        _LOG.warning("could not read %s's source for the framework-key scan "
+                     "(%s); the double-counting guard is NOT enforced for it",
+                     getattr(cls, "id", cls), e)
+        return []
+    for node in ast.walk(tree):
+        body = getattr(node, "body", None)
+        if isinstance(body, list) and body and isinstance(body[0], ast.Expr)                 and isinstance(body[0].value, ast.Constant)                 and isinstance(body[0].value.value, str):
+            body.pop(0)
+    code = ast.unparse(tree)
+    return sorted({name for name in BANNED_IN_PLUGIN_CODE if name in code})
+
+
 PROVIDERS: dict = {}
 
 
@@ -595,6 +694,19 @@ def register(cls):
     if cls.id in PROVIDERS:
         raise ValueError(f"duplicate provider id {cls.id!r}")
     cls.capabilities()          # fail at import, not at 15:00 on fetch day
+    leaks = code_leaks(cls)
+    if leaks:
+        # ⚠️ AT IMPORT, NOT AT FETCH. A plugin that can reach the markup is a
+        # plugin that can bill wrong for a month before anyone notices, so it
+        # does not get to exist - the add-on fails to start and names the line.
+        raise FrameworkOption(
+            f"{cls.id!r} mentions {', '.join(leaks)} in its own code. A plugin "
+            f"returns what its SOURCE published and declares "
+            f"capabilities().sources; the framework adds the complement, once. "
+            f"Reaching any of {BANNED_IN_PLUGIN_CODE} lets it be added twice, "
+            f"and a price that is one markup high is invisible in every log "
+            f"this add-on writes. Remove the reference; if the plugin genuinely "
+            f"needs a per-site number, add a NON-framework option key for it.")
     PROVIDERS[cls.id] = cls
     return cls
 
@@ -682,12 +794,33 @@ def check_egress(url: str, caps: ProviderCapabilities) -> str:
 
     Returns the host, so a caller that forgets to call this is not the same
     shape as one that did.
+
+    ⚠️ THE SCHEME IS SPLIT OFF BEFORE THE HOST IS PARSED, AND THE ORDER MATTERS
+    FOR THE TEST MORE THAN FOR THE CODE. The first version stripped a fixed
+    `len("https://")` from the front, which on an `http://` URL ate two
+    characters of the hostname instead - `http://api.energy-charts.info/p`
+    became the host `pi.energy-charts.info`, which then failed the ALLOW-LIST.
+    So a non-TLS feed was refused, the check named "plain http is refused"
+    passed, and deleting the TLS rule entirely left it passing. Review of
+    PR #16 found that by mutation: an assertion that does not test what it
+    names, inside a security check - this project's own recurring defect. Now
+    the scheme is parsed as a scheme, the host of an `http://` URL comes out
+    correct and DECLARED, and the TLS rule is the only thing that can refuse
+    it - which is what makes the check able to fail.
     """
     if not url:
         raise SourceUnavailable("no feed URL configured (price_feed_url)")
-    if not url.startswith("https://"):
-        raise SourceUnavailable(f"refusing a non-TLS price feed: {url!r}")
-    host = url[len("https://"):].split("/", 1)[0].split("@")[-1]
+    scheme, sep, rest = url.partition("://")
+    if not sep:
+        raise SourceUnavailable(
+            f"not an absolute URL, so no host can be checked against the "
+            f"§16 allow-list: {url!r}")
+    if scheme.lower() != "https":
+        raise SourceUnavailable(
+            f"refusing a non-TLS price feed: {url!r} uses {scheme!r}. §16 "
+            f"allow-lists outbound destinations and SEC-01 requires TLS on "
+            f"every one of them.")
+    host = rest.split("/", 1)[0].split("?", 1)[0].split("@")[-1]
     host = host.split(":", 1)[0].lower()
     if host not in {h.lower() for h in caps.egress_hosts}:
         raise SourceUnavailable(

goodwe_controller/app/validate.py

@@ -435,6 +435,15 @@ def _price_problems(opts: dict) -> list[str]:
     # operator's evidence that they picked a supplier is a status line saying
     # they picked a supplier. That is (a), and it is the "silently double up"
     # the PM named on 2026-08-30.
+    # ⚠️ THIS REFUSAL'S REASON EXPIRES, AND THE MESSAGE BELOW MUST BE EDITED IN
+    # THE SAME COMMIT THAT LANDS THE FIRST CONSUMER (TARIFF-03 or OPT-04).
+    # Today the message says the provider "would decide nothing" - true, because
+    # nothing reads the series. Once something does, both sources really decide
+    # and this becomes a straightforward two-writer conflict; the refusal stays
+    # right and its explanation goes wrong, which is worse than either. There is
+    # a tripwire on exactly this in test_price.py section 12: it greps app/ for
+    # a `load_prices` consumer and goes RED the moment one appears. Update the
+    # message, not the check.
     ha_price = [k for k in ("price_now_entity", "price_avg_entity")
                 if str(opts.get(k, "") or "").strip()]
     if pid != "off" and ha_price: