feat(discovery): observe-only discovery catalogue in the debug dump - #4972
Open
springfall2008 wants to merge 33 commits into
Open
springfall2008 wants to merge 33 commits into
springfall2008 wants to merge 33 commits into
Conversation
Design for a startup discovery phase: components report what they found into a JSON-serialisable catalogue held by a central coordinator, which allocates inverter slots, car charger indices and sensor list indices after phase-1 startup. Automatic configuration then writes apps.yaml keys at the assigned indices instead of racing to overwrite whole lists, which unblocks multi-inverter automatic mode across two components. Spec covers the agreed decisions (serial start retained, the *_automatic keys retained so manual configuration still works, dict-based discovery records so vendor facts ride along). Plan covers 11 TDD tasks: coordinator core, sensor and car allocation, storage persistence, lifecycle plumbing, GivTCP/GECloud/Octopus/Ohme migrations, and a two-vendor headline test. No code changes - documents only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The docs contain a few concrete inconsistencies/typos (e.g., expected inverter_type ordering vs priority allocation, Coordinator signature mismatch, and minor wording issues) that could mislead implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an agreed design spec and a detailed TDD implementation plan for introducing a startup “discovery coordinator” that collects component discovery into a JSON-serialisable catalogue and allocates inverter/car/sensor indices to unblock multi-vendor automatic configuration (e.g., GivTCP + GECloud) without list-overwrite races.
Changes:
- Add a design spec describing the discovery catalogue schema, coordinator responsibilities, and allocation rules.
- Add an implementation plan broken into 11 TDD tasks (coordinator core → persistence → lifecycle plumbing → component migrations → headline integration test → docs).
File summaries
| File | Description |
|---|---|
| docs/superpowers/specs/2026-09-06-discovery-coordinator-design.md | Design spec for the coordinator, catalogue schema, allocation rules, and testing goals. |
| docs/superpowers/plans/2026-09-06-discovery-coordinator.md | Step-by-step TDD plan for implementing the coordinator and migrating components, including test scaffolding and acceptance criteria. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Replaces slot allocation with a first release that only describes what components found: inverters (solar and/or battery), chargers, cars, meters carrying tariffs, forecast providers and flexibility programmes, assembled into one catalogue published in the debug YAML dump. Nothing is allocated and no existing behaviour changes, so real user topologies can be read before allocation rules are designed. Entity descriptors are keyed by Predbat's standard control name and carry min/max/step/options, formalising in band the metadata that today travels as HA entity attributes and is only read back in three places. Redaction is specified up front and applied at source, because the dump is posted to public issues. The generic masker cannot be used: is_secret_key matches the substring "_key", so it would redact device_key/meter_key while passing mpan, account and site_id through in the clear. Fields are allow-listed with a declared class each - never included, pseudonymised, coarsened or clear - keeping serials and tariff codes readable while supply point and account identity become salted tokens that stay correlatable within a dump. Marks the 2026-09-06 allocator spec and plan as deferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The allow-list of individual field names would need editing every time a component learns a new fact, which is friction that gets bypassed. Classify by typed container instead: hardware_ids and info stay clear, account_ids is pseudonymised whatever it holds, ratings takes numbers only, vocabulary lists take short lowercase tokens. A component adds facts freely by picking a container and needs no schema change. Safety now comes from the type constraints rather than from a field census: a numbers-only container and a token-only container cannot carry a name, an email, an address or a credential however the catalogue grows. The two existing guards stay on top - the is_secret_key name check, and value-shape detection for the one gap types cannot close, since an MPAN is a number and so satisfies ratings. Only the handful of top-level structural fields remain enumerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ten TDD tasks: container validation, assembly with per-component status and observed conflicts, redaction with salted pseudonyms, lifecycle wiring into startup and the debug dump, then reporters for GivTCP, GE Cloud, Octopus, Ohme and Solcast, and an end-to-end pass. The release invariant gets its own test: snapshot args, run every reporter, assemble, redact and publish, then assert args is unchanged. Nothing in this version may alter configuration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the coordinator.py foundation for the discovery catalogue: typed containers (hardware_ids, account_ids, info, ratings, coverage, entities, and the vocabulary-list containers), per-container value cleaning, and a Coordinator that validates and stores one component's report at a time. Nothing here allocates anything or writes to self.args. The is_secret_key() credential guard now applies uniformly to every container - including entities descriptor keys and vocabulary tokens, not only the CONTAINER_SPEC dict containers - per the governing design spec's "any container" requirement.
…ard non-dict reports
Closes three review findings on the discovery catalogue coordinator:
- entities descriptor fields (unit, device_class, min/max/step/precision,
options, domain, access, format) were passed through unvalidated -
entities is a "clear" container republished unredacted into debug
dumps, so each field now goes through its own typed cleaner
(DESCRIPTOR_FIELD_CLEANERS). options gets a new bounded-string list
cleaner (capped at 256 entries) rather than the vocabulary-token
pattern, since real select options ("00:30", "PauseCharge") aren't
lowercase tokens.
- coverage was numbers-only; the design spec and an upcoming Solcast
reporter need it to also carry lists of vocabulary tokens (forecast
variants like pv10/pv50/pv90), so it now accepts a number, a boolean,
or a token list via _clean_measure_or_tokens.
- validate_report() dereferenced report.get(...) unconditionally,
raising AttributeError on a non-dict report and contradicting the
documented "never raises" contract; it now treats a non-dict report
as empty and logs a warning instead.
Adds covering tests for all three, plus one making explicit that the
credential guard already fires for a key nested inside a sub-record's
container (tariff.info.api_key), not only at record level.
…ved conflicts Merges every component's discovery report into one site-level catalogue (assemble()), derives a status per registry entry (ok/no_report/not_started/ load_error/not_configured), and records observed conflicts (duplicate serial, multiple inverter sources, multiple import meters, contested car slots) without resolving them, alongside the resulting apps.yaml config for comparison.
Adds Redactor and Coordinator.catalogue()/catalogue_raw()/load_salt() so the discovery catalogue redacts itself before it can reach a public debug dump. Pseudonym/clear container sets are derived from CONTAINER_SPEC's own class tags rather than hardcoded, so a container added later (as entities already was) is handled automatically. The shape guard recurses into descriptor dicts and vocabulary lists rather than stringifying a whole structure, so an identifier misfiled into a nested field (e.g. an entity descriptor's "max") is still caught. A record's device_id is noted alongside its account_ids values so meter/measures_meter cross-links resolve to the matching token even when device_id does not happen to embed the raw identifier as a substring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d substitution
Three root causes from review, addressed as coherent fixes rather than
patch-by-patch:
- Shape guard (root cause A): _misfiled now searches a separator-stripped
string for a 10+ digit run instead of anchoring against the bare value,
so a float ("1234567890123.0"), an embedded identifier ("MPAN
1234567890123"), and one broken up by hyphens, dots, slashes, commas or
underscores are all caught. A location-named key (latitude/longitude/
lat/lon/postcode) is pseudonymised regardless of its value's shape,
since a lat/lon pair cannot be recognised from one value alone.
- Substitution (root cause B): now rewrites dict keys as well as values,
matches non-string scalars by exact equality, and routes every
structural-field scalar (device_id, serials, meter, measures_meter, ...)
through the same shape guard as a clear container's values. To stop this
widening from over-reaching, a noted original is only substring-matched
when it is a genuine identifier (account_ids values, shape/key-guard
catches); a bare device_id noted purely for cross-linking is matched by
whole-string equality only, and originals are substituted longest-first
so a short one cannot fragment a longer one it is a substring of. The
catalogue's own "generated" timestamp is restored verbatim after
substitution, since it is stamped by assemble() itself and can only ever
coincidentally collide with an unrelated original's digits.
- Salt exposure (root cause C): "coordinator" added to utils.DEBUG_EXCLUDE_LIST
as defence in depth against a future refactor exposing it via the debug
dump or MCP state query.
An adversarial pass constructing documents to smuggle an identifier past
the new code found one more gap fixed here (an identifier used as a dict
KEY, e.g. hardware_ids keyed by a raw serial-shaped string, was neither
shape-guarded nor substituted) and one genuine detection hole in the
reviewer's own separator set (underscore- and comma-grouped digit runs),
closed by widening it. 20 new/updated tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ened
Round-2 review confirmed all thirteen prior findings closed and validated
the adversarial pass, but the widening over-corrected in six new ways -
the classic failure mode when a security boundary is broadened. Fixed as
the same three-root-cause groupings, this time narrowing rather than
widening:
- Shape guard (NEW-1, Critical): a numeric value is now judged on its
INTEGER PART's digit count (int(abs(value))), not its stringified repr,
so a legitimate float measurement (1/3, an efficiency, a unit
conversion) survives and stays numeric instead of being pseudonymised
into a string. int(1234567890123.0) is still 13 digits, so a genuinely
misfiled float identifier is still caught.
- Substitution (NEW-2/NEW-3/NEW-5, Important): a numeric identifier noted
in one representation (string/int/float) now registers every
equivalent form (_numeric_variants), so an account_ids value echoed as
a float elsewhere is still caught. Dict keys are now rewritten by exact
match only, never substring - substring-matching keys let an ordinary
account_ids value like "charge" corrupt the "chargers" section name and
the "charge_rate" entity key; _guard_key (shape-based) still catches a
key that IS itself an identifier. A record's device_id is now
substring-eligible (not just whole-string) whenever that record also
carries account_ids, since its identity is then derived from a real
identifier and leaving it whole-string-only let the same text survive
in the clear right next to its own token elsewhere in the record; a
device_id with no account_ids alongside it is still never noted at all,
so an ordinary word used as one still cannot corrupt unrelated text.
- hardware_ids (NEW-4, Important): the digit-run guard inside hardware_ids
now fires only when a value is nothing BUT digits, since a
letter-prefixed serial with a long digit tail ("HV2160123456") is that
container's entire declared purpose and was being over-pseudonymised.
A bare all-digit string there is still caught.
Two tests were rewritten because they did not test what they claimed:
the fragmentation-ordering test used two identifiers the shape guard
already caught directly, so it passed even with the ordering fix
reverted; and the timestamp test never asserted redaction actually
happened, so it would pass against a no-op redact(). Several other tests
gained a "legitimate neighbour survives" assertion alongside the
"identifier is hidden" one - which is exactly what would have caught
NEW-1 and NEW-4 sooner. A follow-up adversarial pass targeting the
narrowed guards found no new leaks; the residual trade-offs it surfaced
(a fractional-only float, a letter-prefixed identifier inside
hardware_ids, an identifier embedded in a component-chosen key) are
inherent to the fixes as specified and documented rather than patched
around.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, not just a sibling Closes the one substantive finding from round-3 re-review, plus two smaller test-quality items flagged alongside it: - ITEM 1: _walk's device_id precondition only checked the record's own top-level keys for a pseudonym container, so a record whose account_ids sits inside a sub-record (a meter's tariff, which _validate_record structurally accepts even though v1's own Octopus reporter puts account_ids at meter level) never had its device_id noted at all - neither whole-string nor substring. The device_id itself, a free-text echo of it, and a cross-link pointing at it all published raw. Fixed by recursing the precondition (_has_pseudonym_container) into every nested dict/list rather than checking only the record's direct keys. - ITEM 2: two round-3 tests (the legitimate-float survival test and the device_id-without-identity-data test) asserted only that nothing changed, with no positive assertion that redaction happened elsewhere in the same document - the same no-op-redact() blind spot fixed in the timestamp test the round before, applied inconsistently. Both now assert a genuine identifier elsewhere in the same catalogue WAS redacted. - ITEM 3: the fragmentation-ordering test's regression-detection power was hash-seed-dependent (a 2-element chain only catches a reverted sort about half the time, since raw `set` iteration order is PYTHONHASHSEED-randomised). Widened to a 4-element nested chain, cutting the "accidentally still passes" probability roughly to 1-in-4, with a docstring note recording that this is a detection-power improvement, not a determinism guarantee - production behaviour (sorting by length, an all-distinct key) is already fully deterministic regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ug dump and an entity Components.coordinator owns the Coordinator (never a PredBat attribute - it holds the raw reports and pseudonym salt, and create_debug_yaml() dumps every non-excluded PredBat member). ComponentBase gains component_name (registry key, set by Components.initialize()) and report_discovery(), a no-op without a coordinator so the standalone CLI harnesses are unaffected. PredBat.initialize() assembles and publishes the catalogue as sensor.predbat_discovery right after phase-2 start, and create_debug_yaml() adds a redacted "discovery" key - both call sites are exception-wrapped so an observer can never break startup or a bug report. Also folds in a reported_at UTC timestamp per component (Coordinator.report()), since the field existed in _component_status but was never populated, which would have shown reported_at: null forever in every published catalogue. create_debug_yaml() reads self.components.coordinator via getattr rather than a bare attribute access - a components stand-in that does not model .coordinator (several existing tests use one) would otherwise crash the debug dump outright instead of degrading gracefully.
…whole catalogue publish() previously folded the entire redacted catalogue into sensor.predbat_discovery's attributes via attributes.update(catalogue), including every inverter's full entity-descriptor map - tens of kilobytes of JSON that the HA recorder stores to disk for an ordinary install. The full document already reaches its intended consumer through the debug dump; the (future, out-of-scope) web viewer should ask the coordinator directly rather than parse it back out of an entity's attributes. publish() now carries only schema_version, generated, a per-section count map, the components status map, and observations.conflicts - never a section's record list. The entity state is unchanged (the total record count).
Adds GivTCPComponent.build_discovery(), called from run() alongside the existing automatic_config() trigger and gated on a new self.reported_for so a fleet that grows on a later re-probe reports again. One record per endpoint that actually answered discovery, with entity descriptors built from GIVTCP_CONTROLS/GIVTCP_SENSORS (the real per-device rate maximum overriding the generic ceiling) and capabilities drawn from the same probes automatic_config() already gates its own decisions on. Reporting is independent of self.automatic - the catalogue describes what hardware is physically there. automatic_config() itself is unchanged.
…guard the report call build_discovery() previously described every GIVTCP_CONTROLS/GIVTCP_SENSORS entry unconditionally, but publish_data() gates many of them (pause control, discharge_target_soc, charge_limit_enable, the scheduled_*_enable switches, most discovery/energy sensors) behind GivTCP version, register support or a non-None reading. That let the catalogue claim an entity existed - e.g. pause_mode on a v2 install - when no such HA entity was ever created. _discovery_descriptor now checks the state store (get_state_wrapper) rather than re-deriving publish_data()'s own gating a second time, since publish_data() always runs earlier in the same run() cycle and its output is already the ground truth. Also wraps the new report_discovery() call in run() in a try/except, matching publish_data()'s own per-inverter guard: an observer must not be able to degrade the health of the thing it observes by crashing run() and withholding update_success_timestamp() every cycle. self.reported_for is left unset on failure so the next cycle retries.
The report block ran after rediscover(), which appends a newly-found endpoint to self.discovered without publishing anything for it - publish_data() already ran earlier that same cycle, over the old self.discovered. That let a rediscovered inverter be reported (and self.reported_for advanced) with an empty entity map, on the cycle it was found, and since self.reported_for had already advanced, the correct entities publish_data() produced sixty seconds later were never retried. Move the report block above "if rediscover:"/automatic_config(): on the rediscovery cycle itself self.discovered hasn't grown yet, so the report is a no-op; the following cycle's poll republishes the grown fleet, including the rediscovered inverter's real entities, before this now-mismatched report block runs again. Added a test that drives an actual run() cycle through rediscovery (not a direct build_discovery() call, which sidesteps the ordering that broke) and asserts the rediscovered inverter's report only fires - with a populated entity map - once publish_data() has actually published it. Verified the test fails against the pre-fix ordering and passes against the fix.
…talogue GECloudDirect.build_discovery() describes discovered battery/PV devices for the discovery catalogue: composition (direct/gateway/ems) mirrors the exact precedence async_automatic_config() applies without modifying it, capabilities are sniffed per-device from self.settings rather than the aggregate flags automatic_config() uses, and measures_meter cross-links two devices sharing a CT clamp serial to the same meter record. Reported from the existing "if first:" block, guarded by try/except so a reporting bug can never withhold run()'s success timestamp, and independent of self.automatic per the design.
…eter records Two review findings on the GE Cloud discovery reporter: - The report only ever ran inside "if first:", which flips permanently False the moment run() returns True - a build_discovery() failure swallowed by the try/except (so the component stays healthy) meant the report was silently lost for the life of the process, unlike async_automatic_config() which is unguarded and gets a de facto retry from ComponentBase.start() itself. Added self.discovery_reported_for, compared against self.devices_dict on every pass through the settings block (not gated on "first"), so a failed attempt is retried once the underlying problem clears. - build_discovery() invented a "meters" record for each CT/meter serial so measures_meter had somewhere to point. A meters-section record in this design is a utility supply point (direction, MPAN, tariff); a CT clamp is hardware measuring a circuit, not a billing point, and doesn't fit that identity model - GivTCP, the reference reporter, reports no meters key at all for the same reason. measures_meter is kept as a deliberately dangling cross-link; "meters" is now always returned empty.
A component that knows a hardware meter serial but has no utility supply point to describe must report no meters records at all, rather than inventing one so measures_meter resolves. A CT clamp attached to an inverter is measurement hardware, not a billing point, and giving it a meters record lets it collide with real supply points in the multiple_import_meters detector - today only avoided because such a record happens to carry no direction. A dangling cross-link is an honest record of what is known; resolving it belongs to the observation layer, not the reporter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the catalogue Adds OctopusAPI.build_discovery(), reporting one meters record per direction present in self.tariffs (mpan/account pseudonymised via account_ids, tariff/product codes and classifier flags in the clear) and one cars record per active intelligent device, with entities included only once they actually exist in the state store. Reported beside each of automatic_config()'s three existing call sites via a small helper that compares a state-key marker outside any one-shot gate and guards the call with try/except, so a transient failure or an incomplete car-entity set is retried rather than lost. automatic_config() itself is unmodified.
…ctopus regardless of gating Task 7 review found two issues: Finding 1 (critical): a vendor helper shaped like Octopus's get_entity_name() lower-cases an identifier and swaps "-" for "_" when folding it into an entity id, producing a string that is neither equal to, nor a literal substring of, the noted pseudonymised original - so it reached the redacted catalogue in the clear. Fixed generically in Redactor._note() via a new _identifier_variants() helper, composed with the existing numeric-variant mechanism, so every future reporter (not just Octopus) is protected. Finding 2 (important): OctopusAPI's discovery report was only reachable from three narrow, largely one-shot branches beside automatic_config() - for a stable installation (unchanging tariff and device set) none of them fire again after the first cycle, so a first-cycle build_discovery() failure was never retried, and an octopus_automatic: false installation was never reported on at all. Fixed by calling _refresh_discovery_report() unconditionally once per run() cycle, outside "if self.automatic:", in addition to the existing call sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds OhmeAPI.build_discovery(), reporting one chargers record and one cars record (non-stub with make/model when a current vehicle is known, else a stub), cross-linked via serves_cars/charged_by. Reported regardless of ohme_automatic via an unconditional call in run(), outside the one-shot "if first and self.client.serial" gate, so a transient failure is retried rather than lost for the life of the process (the bug GE Cloud and Octopus both shipped and had to fix). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up: adds a test driving a real run() cycle before the client has a serial, asserting build_discovery()/report_discovery() are not called and the marker does not advance - the early-return guard in _refresh_discovery_report() was already correct but untested. Also fixes CHARGER_DISCOVERY_ENTITY_SPEC's comment, which still said "four" after car_charging_now was cut to three entries, and hoists the duplicated run()-stubbing helper used by the three run()-level discovery tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the forecasts section's first reporter: one record per Solcast resource id, forecast.solar, Open-Meteo, and the external HA-sensor fallback path, each carrying its own coverage (horizon, resolution, and - for Solcast - the pv10/pv50/pv90 variant list). Solcast site ids are pseudonymised via account_ids; site names are never read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… provider Review follow-up: forecast.solar/Open-Meteo carry a real, already- configured kwp per plane (unlike Solcast, which has no local capacity fact), so their forecasts records now report ratings.capacity_kw rather than omitting a genuinely known figure. Also add ratings.active, marking whichever record's provider actually served the most recent successful fetch, so a stale HA-sensor config alongside a live Solcast setup no longer looks indistinguishable from the real source in the catalogue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fetch_pv_forecast()'s configured_source local keeps naming the primary provider even once an internal fallback (Open-Meteo <-> forecast.solar) has actually served the data, so active_forecast_source could point a maintainer at the wrong integration. Track which provider genuinely returned data via a second local (configured_source itself, and the precedence logic, are untouched) so ratings.active always names the real source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… invariant Wires GivTCP, GE Cloud, Octopus, Ohme and Solcast into one shared MockBase/Coordinator fleet and proves the release's central promise - that running the full discovery path (build_discovery/report_discovery/assemble/redact/publish) changes not one byte of apps.yaml - alongside an end-to-end assembly test across every section and conflict kind, and a redaction corpus test covering original and transformed leak forms. Adds docs/discovery-catalogue.md (user-facing redaction guarantees plus the reporter contract for developers) and registers the new test module in unit_test.py's TEST_REGISTRY. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…id pseudonymisation
hardware_ids alone requires a value to be nothing but digits before it is flagged; every
other clear container catches a 10+ digit run embedded anywhere in a longer string, which
the prior wording didn't distinguish. Also notes that a device_id built from a sensitive
identifier (e.g. Octopus's "octopus:{mpan}") is replaced wholesale by its pseudonym token,
losing its prefix - a visible, harmless inconsistency a reader of a dump would otherwise
have to wonder about. Adds "pseudonymisation" to the cspell workspace dictionary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…'s own status Every reporter's discovery-report guard (GivTCP, GE Cloud, Octopus, Ohme, Solcast) called self.non_fatal_error_occurred() on failure, which sets base.had_errors. The had_errors branch in update_pred() skips record_status() entirely - freezing predbat.status on its previous value and suppressing the run notification - and also bumps the errors_total Prometheus counter. So a bug in this purely observational side channel could change Predbat's user-visible state, breaking the release's "cannot change how any component behaves" promise. solis.py already documents this exact trap. Remove the non_fatal_error_occurred() call from all five guards, keeping the log line. Flip the three tests that had asserted had_errors was set (GivTCP, GE Cloud, Solcast) to assert it is not - a discovery failure is now visible only in the log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…and two redaction gaps Coordinator._note() derived a value's pseudonym token from whichever textual form it was handed and overwrote any earlier mapping for a shared variant, so the same real-world identifier noted twice in different forms (a string MPAN vs. its float echo; "AB-12CD34" vs. "ab_12cd34") produced two different, order-dependent tokens - defeating the cross-record/cross-dump stability the design relies on. _note() now builds the full variant set first and reuses whichever token any variant already maps to. Coordinator.catalogue()/catalogue_raw() returned `self.assembled or self.assemble()`, and assemble() was only ever called once at the startup barrier - so a report filed afterwards (a retry, a rediscovered inverter, a changed tariff or vehicle, a later-discovered forecast site) could never reach a debug dump. Both now re-assemble on every call; assemble() is cheap enough (a lock briefly held to copy two dicts, then a merge over a handful of lists) that this costs nothing measurable. Extended test_debug_yaml_scope.py's _DiscoveryComponentsStub with the registry surface _component_status() now exercises on every read, since it previously only had to support one assemble() call while my_predbat.components was forced to None. An account_ids value equal to a structural name byte-for-byte (e.g. "chargers", or "account_ids" itself) was rewritten by _substitute_key's exact-match pass exactly like any other key, silently deleting that whole top-level section or container from the published catalogue. PROTECTED_KEY_NAMES now excludes every SECTION_SPEC/CONTAINER_SPEC name plus "components"/"observations" from that pass. _guard_key logged the raw key text it had just decided to pseudonymise - Predbat's log file is attached to the same public issues as a debug dump, so this republished exactly what the guard exists to hide. Now logs only the container name, matching _guard_scalar's existing behaviour. _identifier_variants() only folded case DOWN, so an upper-cased echo of a lower-cased canonical identifier survived redaction. Now registers both fold directions. Octopus's Intelligent-device car record carried no account_ids, so a UUID-shaped device id (a genuine per-account identifier, not a public one) reached the redacted catalogue verbatim - the same class of leak Solcast's own provider id is deliberately protected against. The car record now carries the device id in account_ids too. Also rolls in two small coordinator.py cleanups that touch the same code this commit already changes: the orphaned DESCRIPTOR_FIELDS tuple (replaced by DESCRIPTOR_FIELD_CLEANERS, unused) is deleted, and "automatic" is now carried through only when a component's own report actually provides it, rather than defaulting a component with no such concept (Solcast) to `automatic: true` in the catalogue. Tests added throughout test_coordinator.py and test_octopus_intelligent_devices.py for every fix above; docs/discovery-catalogue.md updated to describe live re-assembly instead of the old "assembled once at startup" behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…coverage ratings is specced for physical quantities (capacity, efficiency, ...); coverage.active is a status flag, and coverage's own container type already accepts a boolean (Coordinator._clean_measure_or_tokens), so it validates cleanly there without any coordinator.py change. Moves all four call sites (the per-site Solcast record, forecast_solar, open_meteo, ha_sensors) and updates the seven test assertions in test_solcast.py that read ratings.active. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
springfall2008
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Conflict detection, meter identity handling, report refresh behavior, malformed input handling, and error redaction contain unresolved correctness or security issues.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 5
- Review effort level: Balanced
Comment on lines
+1452
to
+1453
| mpan = self.mpan if direction in ("import", "export") else None | ||
| record = {"device_id": "octopus:{}".format(mpan or direction), "direction": direction} |
Owner
Author
There was a problem hiding this comment.
Automated reply from the triage bot.
Confirmed and fixed. self.mpan is set at octopus.py:866 only from the first active import meter's meterPoint, and an export agreement is a separate electricityAgreements entry with its own meterPoint.mpan - so both records got the import MPAN, and (since device_id is "octopus:{mpan}") both got the same device_id too. One supply point where the account has two.
async_find_tariffs() now records mpan per direction off the agreement it is reading, and build_discovery() uses that. self.mpan is kept as the fallback for import alone, the one direction it is known to describe - an export agreement with no MPAN reports no mpan rather than borrowing the import one. Also added to _discovery_state_key() so a later-arriving MPAN re-reports.
Covered by test_find_tariffs_records_each_agreements_own_mpan (the two-agreement account through the real async_find_tariffs) and test_build_discovery_meters_carry_their_own_direction_mpan.
Comment on lines
+633
to
+640
| serials = {} | ||
| for record in catalogue["inverters"]: | ||
| serial = record.get("hardware_ids", {}).get("serial") | ||
| if serial: | ||
| serials.setdefault(str(serial).casefold(), set()).add(record["source"]) | ||
| for serial, sources in sorted(serials.items()): | ||
| if len(sources) > 1: | ||
| conflicts.append({"kind": "duplicate_serial", "serial": serial, "claimed_by": sorted(sources)}) |
Owner
Author
There was a problem hiding this comment.
Automated reply from the triage bot.
Confirmed and fixed. gecloud.py:1631 puts the fronted battery serials in the structural serials list and the gateway's own serial in hardware_ids, while GivTCP reports each battery's own serial in hardware_ids - so the two never shared a top-level serial and duplicate_serial stayed silent on the one collision the observation exists to count.
_conflicts() now groups on every serial a record speaks for: hardware_ids.serial plus each entry in serials. A gateway alone still emits nothing, since the claims are grouped into a set of sources and one component cannot conflict with itself. multiple_inverter_sources already fired for this case, but only at fleet granularity - it does not say which battery is double-claimed.
Covered by test_observations_duplicate_serial_sees_a_gateways_fronted_batteries (both halves), and the conflict table in docs/discovery-catalogue.md now says so.
Comment on lines
+837
to
+840
| for section in SECTION_SPEC: | ||
| records = [] | ||
| for record in report.get(section, []) or []: | ||
| validated = _validate_record(record, section, component_name, log) |
Owner
Author
There was a problem hiding this comment.
Automated reply from the triage bot.
Confirmed and fixed. report.get(section, []) or [] returns the scalar unchanged for {"inverters": 1}, so the for raised TypeError straight out of report() - against a docstring that promises "never raises". A bare string was the quieter version of the same bug: it iterated as characters, one dropped-record warning per character.
validate_report() now rejects a non-list section value before iterating it, logging the type it got. The drop is per section, not per report: a valid section alongside a malformed one still survives, which matches how a malformed record is already handled.
Covered by test_validate_report_never_raises_on_a_non_list_section, which pins both the scalar case and the mixed valid/invalid case.
Comment on lines
+506
to
+509
| if self.discovered != self.reported_for: | ||
| try: | ||
| self.report_discovery(self.build_discovery()) | ||
| self.reported_for = list(self.discovered) |
Owner
Author
There was a problem hiding this comment.
Automated reply from the triage bot.
Confirmed and fixed - this was the most consequential of the round. build_discovery() reads rest.serial_number, rest_v3, the pause probes, battery_soh(), charge_target_enabled, firmware and GivTCP version, and the conditionally-published entity map. None of those moves self.discovered, so an endpoint that answered before GivTCP had decoded its inverter registers had its URL-derived device_id and missing hardware_ids frozen for the life of the process - which is exactly the identity a dump gets read for.
self.reported_for is replaced by self.reported_report, the last report that filed successfully, compared whole. That is the "(or the built report)" half of your suggestion rather than a separate input snapshot, which would have been a second copy of build_discovery()'s own input list to keep in step. run() is called once a minute and the build is dict lookups over already-read REST data, so rebuilding to make the comparison costs nothing measurable, and report_discovery() is still only reached when something actually moved.
The rediscovery ordering the block's comment describes is unchanged: on the cycle rediscover() grows the fleet, the report built before it still equals the last one and is a no-op, so the rediscovered inverter is still reported only once publish_data() has published its entities. test_rediscovered_inverter_is_reported_only_once_its_entities_exist still passes as-is. New coverage in test_a_partial_first_report_is_replaced_once_the_inverter_fills_it_in, which also pins the other half - an unchanged cycle must not re-file, or every install would churn the catalogue once a minute.
springfall2008
added
the
BOT_CLEANUP
…nator # Conflicts: # apps/predbat/tests/test_givtcp_component.py
…elf-correcting GivTCP reports Addresses the review round on the observe-only catalogue: - octopus: each meter record carries its own agreement's MPAN. self.mpan is the import supply point only, so export published the import MPAN as its identity and both records shared one device_id. - givtcp: compare the built report rather than the discovered index list, so a first answer missing a serial, firmware or a conditional capability is replaced once the endpoint supplies it instead of being frozen for the life of the process. - coordinator: a gateway's fronted battery serials take part in duplicate_serial, which is what makes the GE Cloud gateway + GivTCP double-claim visible at all. - coordinator: a non-list section is dropped rather than iterated, restoring validate_report()'s "never raises" contract for a malformed section. - coordinator: a load_error status no longer republishes the raw exception text - the one free-text field outside the typed-container boundary. The message stays in the log and on the component-status entity. - docs: correct the deferred allocator spec's slot order (GE Cloud precedes GivTCP in COMPONENT_LIST) and drop a transient note from the plan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
springfall2008
removed
the
BOT_CLEANUP
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
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.
What this does
Components now describe what they discovered — inverters, chargers, cars, meters with their tariffs, and forecast providers — into a single catalogue that is assembled, redacted, and written into the debug YAML dump as a
discovery:section, plus a summary onsensor.predbat_discovery.Nothing acts on it. No slots are allocated, no
self.argskey is written, and no component's behaviour changes. The point of this release is to be able to read real user topologies from bug reports before designing any allocation rules — how often two inverter components are enabled together, how often a serial is claimed twice, whether sensor-only PV devices are common enough to justify a separate index pool. Those questions have no answer today, and guessing at them is how the allocator would get designed wrong.Five components report: GivTCP and GE Cloud (inverters), Octopus (meters, tariffs, intelligent-device cars), Ohme (charger and car, as separate records), Solcast (forecast providers).
The observe-only guarantee
test_discovery_catalogue.pysnapshotsargs, runs every reporter's real discovery path, assembles, redacts and publishes, then assertsargsis byte-for-byte unchanged. Both new call sites — the startup barrier ininitialize()and the line increate_debug_yaml()— are exception-wrapped, so a failure in the observer cannot break startup or break a bug report.What is and is not redacted
Dumps get attached to public issues, so the catalogue redacts itself at source rather than relying on a caller to remember.
Pseudonymised with a per-installation salt: MPANs, account numbers, site/plant/system ids, hub serials, login identifiers. Every textual form of a value maps to one token, so cross-links still resolve within a dump and across successive dumps from the same install, while the same MPAN from two different users does not collide.
Kept readable, deliberately: device serials, model, firmware,
inverter_type, capabilities, ratings, tariff and product codes, entity ids. These are what make an integration bug diagnosable, and the codebase already draws this line —secret_config_names()does not flag serials.Safety comes from typed containers rather than a field allow-list: a container that accepts only numbers cannot leak a name however the catalogue grows, so adding a fact means picking a container, not editing a census.
Where to look
docs/discovery-catalogue.md— user-facing, including what is redacted so a user knows a dump is safe to attach, and the four rules a new reporter has to follow.docs/superpowers/specs/2026-09-10-discovery-catalogue-design.md— the design and its rationale.docs/superpowers/specs/2026-09-06-discovery-coordinator-design.md, unchanged and marked deferred.Testing
Full suite green: 1050 tests. Coverage includes container validation and its drop-on-mismatch behaviour, assembly and component-status derivation, conflict detection, a redaction corpus seeded with a credential, an MPAN, an email, a postcode and coordinates (asserting transformed forms, not just raw strings), per-reporter tests, and the observe-only invariant.
Known limitations
observations.conflictsrecords collisions — duplicate serials, two inverter sources, contested car slots — without resolving them. That is deliberate for this release.metersrecord rather than inventing one.hardware_idscarrying a non-digit character; and an identifier embedded inside a component-chosen dict key.ComponentBasehelper would unify them and is worth doing before a sixth reporter lands.test_github.pysetsmy_predbat.componentsat two call sites without restoring it.🤖 Generated with Claude Code