Merge remote-tracking branch 'origin/main' into fix/issue-630-subfloor-solar-export

Author: johanzander

.claude/skills/implement-issue/SKILL.md

@@ -58,7 +58,7 @@ runners — only repo-level `.claude/skills/` and `.claude/agents/` exist there.
 
 | Step | CI mode |
 |---|---|
-| 0. Resume check | Applies, and matters more here: Stage 3 is re-triggered by hand, so a second `@claude-bot fix` on an issue that already has a `has-fix-pr` PR is a resume, not a restart. Detect the existing PR and continue it — never open a second PR for one issue. The CI checkout has no worktrees, so branch existence on `origin` is the only signal available. |
+| 0. Resume check | Applies, and matters more here: Stage 3 is re-triggered by hand, so a second `@claude-bot fix` on an issue that already has a `has-fix-pr` PR is a resume, not a restart. Detect the existing PR and continue it — never open a second PR for one issue. The CI checkout has no worktrees, so branch existence on `origin` is the only signal available. Reading the PR's **conversation comments** matters more here too, not less: the re-trigger is itself a comment, so the maintainer has very often said *why* in the same thread. |
 | 2. Diagnose | Stage 2 comment absent → STOP. Post "No deep analysis found. Run `@claude-bot analyze` first" and exit — never self-diagnose in CI; the analyze/fix split *is* the human gate. |
 | 3. Confirm gate | The owner's `@claude-bot fix` comment is the go-ahead. Still perform the workaround check and scope assessment — put them in a `## Scope assessment` section of the PR body instead of chat. Escalation path (can't confidently pass the workaround check) still applies: dispatch a fresh general-purpose `Agent` to critique the design before implementing. |
 | 4. Worktree | Skip — the CI checkout is already isolated. Create the branch directly (naming per Step 1). |
@@ -85,10 +85,14 @@ for `TODO.md` items and for refactors that never had an issue, so a PR with no
 linked issue is the normal shape for that work, not a defect.
 
 ```bash
-gh pr view <n> --json number,headRefName,isDraft,mergeable,reviews 2>/dev/null \
+gh pr view <n> --json number,headRefName,isDraft,mergeable,reviews,comments 2>/dev/null \
   || gh issue view <n> --json number,title,labels,body,comments
 ```
 
+**`comments` is in that list deliberately — do not drop it.** It is a separate
+feed from `reviews`, it is where the maintainer sets direction, and omitting it
+has already cost one full rework (see Rehydrate, below).
+
 If `<n>` is a **PR**, resume from it directly — it is the stronger handle,
 carrying the branch, the diff, the `## Scope assessment` and the review verdict,
 which is everything the table below reads. Read its linked issue too if it
@@ -127,8 +131,38 @@ this skill already forces it to be written down:
   discriminate. Interactive mode keeps its scope assessment conversational, so
   that heading is absent on most PRs this skill opens; the PR's existence is
   what proves Step 9 was reached, not any particular heading
+- **the PR CONVERSATION comments, not only its reviews** — see below
 - the diff itself, and any inline review comments
 
+**Reviews and conversation comments are two different feeds, and `gh pr view
+--json reviews` returns only the first.** Read both, every time:
+
+```bash
+gh pr view <n> --json reviews  --jq '.reviews[]  | "\(.author.login) \(.state) \(.submittedAt)\n\(.body)"'
+gh pr view <n> --json comments --jq '.comments[] | "\(.author.login) \(.createdAt)\n\(.body)"'
+```
+
+**A maintainer conversation comment OUTRANKS every bot review on the PR**,
+including ones submitted after it. The bot reviews the diff; the maintainer
+decides the direction, and they change direction in comments — that is the
+only place they can, since a review has to attach to a diff.
+
+This is not hypothetical. On #620 the maintainer posted "**this PR should
+shrink**" with a four-step plan: branch protection now rejects pushes to `main`
+server-side, so the protected-ref *enumeration* should be **deleted** rather
+than extended. A later session read the reviews, did not read the comments,
+and spent a full rework **adding** protected-ref patterns — the exact opposite
+of the standing instruction, on a PR whose own thread already said so. Two
+further holes the maintainer had found by hand (`git push origin
+refs/tags/v1.2.3`, `git push origin HEAD`) were in that comment too, and stayed
+open because nobody read it.
+
+So: **before touching code on a resumed PR, read the human comments first, and
+newest-first.** If one sets a direction the diff contradicts, that is a
+STOP-and-confirm, not something to reconcile silently — the maintainer may have
+changed their mind since, and asking costs one message where guessing costs a
+rework.
+
 **If those sources do not reconstruct a coherent diagnosis, STOP and report
 it.** Do not re-diagnose from scratch on top of someone else's half-finished
 branch: you would be building on a design you cannot see, and the commits
@@ -674,6 +708,20 @@ On the verdict:
   silently ignore it either.
 - **A review from the maintainer rather than the bot** (the `<author>` field):
   treat it as authoritative and stop the loop — a human has taken over.
+- **A maintainer CONVERSATION comment, which is not a review at all**, is
+  equally authoritative and arrives on a feed the verdict never touches. Check
+  it each round, not only at Step 0 — a direction posted mid-loop is invisible
+  to `gh pr view --json reviews`, and to the bot, which will keep reviewing the
+  diff as if nothing had been said:
+
+  ```bash
+  gh pr view <n> --json comments \
+    --jq '.comments[] | select(.createdAt > "<submittedAt from the round before>") | "\(.author.login)\n\(.body)"'
+  ```
+
+  If one lands, stop the loop and act on it before the next round. A bot
+  APPROVED on a diff the maintainer has already asked you to redo is worth
+  nothing.
 
 Fix the blockers, park genuine nits in `TODO.md`, run
 `./scripts/quality-check.sh`, commit, and push. Step 9's frontend rule carries
@@ -756,6 +804,8 @@ net is upstream, not this section.
 | "the old branch is a mess, cleaner to redo it" | Its commits are the only copy of a diagnosis you no longer have. If you genuinely cannot reconstruct the approach, that is a STOP-and-report, not a licence to reset. |
 | "that worktree's session shows dead, so it's mine to take" | Check unsandboxed. A sandboxed `claude agents --json` returned 1 session where the real answer was 17, because `~/.claude/jobs` is sandbox-denied — every other session read as dead. |
 | "the review said CHANGES_REQUESTED but nobody assigned it to me" | Nothing else will pick it up. Once the opening session exits, an orphaned PR has no owner at all — `sweep-prs` refuses the job by design. Resuming is how it gets one. |
+| "I read the reviews, so I know what this PR needs" | Reviews and conversation comments are separate feeds and `--json reviews` returns only one. The maintainer sets *direction* in comments, because a review can only attach to a diff. On #620 that cost a full rework in the opposite direction. |
+| "the bot approved it, so the direction must be fine" | The bot reviews the diff against a checklist; it has no idea what the maintainer asked for in the thread. An approval on a diff you were told to redo is worth nothing. |
 | "I can see the assertion is right, no need to run it red" | Assertions that look right have repeatedly bounded only one side, or compared a quantity a second varying term swamped. Seeing it fail is the cheap part. |
 | "quality-check.sh passed, that's enough" | Green tests prove the suite is satisfied, not that the fix behaves correctly against the real scenario. Step 8 requires observed output, every time. |
 | "the diagnosis is obviously right, skip the confirm gate" | Wrong diagnoses are exactly when confidence is highest. One message, cheap insurance. |
@@ -787,6 +837,12 @@ net is upstream, not this section.
 - About to re-diagnose from scratch on top of someone else's half-finished
   branch because the Stage 2 comment and PR body didn't reconstruct the
   approach. That is a STOP-and-report.
+- **About to change code on a resumed PR without having read its conversation
+  comments** — not just its reviews. They are separate feeds, and the
+  maintainer's direction lives in the one `--json reviews` does not return.
+- About to implement a diff that contradicts a maintainer comment already on
+  the thread. Stop and confirm; they may have moved on, but guessing costs a
+  rework and asking costs one message.
 - About to open a second PR for an issue that already has one.
 - About to relaunch an issue that has already died twice without saying so.
 - About to commit or open the PR without having actually run/observed the

CHANGELOG.md

@@ -23,6 +23,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Fixed
 
 - **A tiny solar surplus is no longer planned as an export the inverter will absorb** — below the export the plan can express, the battery charged anyway and ran fuller than planned, spilling the difference later. ([#630](https://github.com/johanzander/bess-manager/issues/630))
+- **The setup wizard no longer locks you out of an inverter platform it failed to auto-detect** — every platform stays selectable, and a re-scan keeps the one you picked. ([#621](https://github.com/johanzander/bess-manager/issues/621))
 - **System no longer gets stuck on "initializing" when many consecutive periods are near-tied** — a long run of volatile prices could make every hourly optimization fail, leaving no schedule at all. ([#624](https://github.com/johanzander/bess-manager/issues/624))
 - **Grid charging now reaches the planned amount instead of stopping just short** — the charge rate is written as a whole percent, and rounding it down meant the battery charged slightly less than the plan counted on.
 - **Growatt VPP no longer briefly executes the previous period's power command when switching modes** — enabling remote control commits immediately, so the power target is now written before it, and cleared on release. ([#593](https://github.com/johanzander/bess-manager/issues/593))

backend/api.py

@@ -2962,6 +2962,7 @@ async def run_setup_discovery():
             {
                 "growatt_found": integrations["growatt_found"],
                 "growatt_device_id": integrations["growatt_device_id"],
+                "huawei_found": integrations["huawei_found"],
                 "huawei_device_id": integrations.get("huawei_device_id"),
                 "solax_found": integrations["solax_found"],
                 "solax_has_growatt_tou": integrations.get(

backend/tests/test_setup_api.py

@@ -953,6 +953,7 @@ def test_octopus_only_persists_gbp_defaults(self):
         integrations = {
             "growatt_found": False,
             "growatt_device_id": None,
+            "huawei_found": False,
             "solax_found": False,
             "nordpool_found": False,
             "nordpool_area": None,
@@ -986,6 +987,7 @@ def test_nordpool_discovery_does_not_clear_costs(self):
         integrations = {
             "growatt_found": False,
             "growatt_device_id": None,
+            "huawei_found": False,
             "solax_found": False,
             "nordpool_found": True,
             "nordpool_area": "SE3",
@@ -1019,6 +1021,7 @@ def test_norwegian_nordpool_updates_currency_preserves_costs(self):
         integrations = {
             "growatt_found": False,
             "growatt_device_id": None,
+            "huawei_found": False,
             "solax_found": False,
             "nordpool_found": True,
             "nordpool_area": "NO1",
@@ -1049,6 +1052,7 @@ def test_no_locale_hints_leaves_defaults_unchanged(self):
         integrations = {
             "growatt_found": False,
             "growatt_device_id": None,
+            "huawei_found": False,
             "solax_found": False,
             "nordpool_found": False,
             "nordpool_area": None,
@@ -1080,6 +1084,7 @@ def test_discover_optional_sensors_receives_entity_registry(self):
         integrations = {
             "growatt_found": False,
             "growatt_device_id": None,
+            "huawei_found": False,
             "solax_found": False,
             "nordpool_found": False,
             "nordpool_area": None,
@@ -1108,6 +1113,96 @@ def test_discover_optional_sensors_receives_entity_registry(self):
         )
 
 
+class TestDiscoverForwardsInverterDetectionFlags:
+    """POST /api/setup/discover must forward a detection flag for EVERY
+    inverter platform the wizard shows (#621).
+
+    `discover_integrations()` produces `huawei_found`, but the endpoint
+    dropped it while forwarding the other three. The wizard's
+    `DiscoveryResult` declares `huaweiFound: boolean` (non-optional), so the
+    missing key surfaced as `undefined` rather than a type error, and the
+    Huawei detection dot read grey for every user including a correctly
+    detected stock `huawei_solar` install.
+
+    Asserting the flag on `discover_integrations()` alone is what let this
+    through — `test_scenario_discovery.py::...` already did that and passed.
+    The gap is in the endpoint's payload, so that is what these assert.
+    """
+
+    def _run_discover(self, ctrl, integrations):
+        ha = ctrl.ha_controller
+        ha.discover_integrations.return_value = (integrations, [])
+        ha.fetch_entity_registry.return_value = []
+        ha.discover_sensors_from_registry.return_value = ({}, None, {})
+        ha.discover_current_sensors.return_value = {}
+        ha.discover_optional_sensors.return_value = {}
+        ha.discover_octopus_entities.return_value = {}
+        ha.ENTITY_SUFFIX_MAP = {}
+        ha.SOLAX_GROWATT_MIN_SUFFIX_MAP = {}
+        ha.SOLAX_GROWATT_SPH_SUFFIX_MAP = {}
+        ha.SOLAX_NATIVE_SUFFIX_MAP = {}
+        sys.modules["app"].bess_controller = ctrl
+        return _client.post("/api/setup/discover")
+
+    @staticmethod
+    def _integrations(**overrides):
+        base = {
+            "growatt_found": False,
+            "growatt_device_id": None,
+            "huawei_found": False,
+            "huawei_device_id": None,
+            "solax_found": False,
+            "solis_found": False,
+            "nordpool_found": False,
+            "nordpool_area": None,
+            "nordpool_custom_area": None,
+            "nordpool_custom_entity": None,
+            "nordpool_config_entry_id": None,
+            "octopus_found": False,
+            "detected_inverter_platforms": [],
+            "detected_phase_count": None,
+            "currency": None,
+            "vat_multiplier": None,
+        }
+        base.update(overrides)
+        return base
+
+    def test_every_platform_detection_flag_is_present_in_the_payload(self):
+        """All four wizard platform tabs need their flag, not just three."""
+        ctrl = _make_discover_controller(deepcopy(_PRE_EXISTING_STORE))
+        resp = self._run_discover(ctrl, self._integrations())
+
+        assert resp.status_code == 200
+        body = resp.json()
+        for key in ("growattFound", "solaxFound", "solisFound", "huaweiFound"):
+            assert key in body, f"{key} missing from /api/setup/discover payload"
+
+    def test_detected_huawei_is_reported_as_found(self):
+        """A stock huawei_solar install must light the Huawei dot green."""
+        ctrl = _make_discover_controller(deepcopy(_PRE_EXISTING_STORE))
+        resp = self._run_discover(
+            ctrl,
+            self._integrations(
+                huawei_found=True,
+                huawei_device_id="dev-huawei-1",
+                detected_inverter_platforms=["huawei_solar_luna2000"],
+            ),
+        )
+
+        assert resp.status_code == 200
+        assert resp.json()["huaweiFound"] is True
+
+    def test_undetected_huawei_is_reported_as_not_found(self):
+        """The reporter's case: EMMA integration, so the flag is False --
+        False, not absent. The wizard must still be able to offer the tab.
+        """
+        ctrl = _make_discover_controller(deepcopy(_PRE_EXISTING_STORE))
+        resp = self._run_discover(ctrl, self._integrations(huawei_found=False))
+
+        assert resp.status_code == 200
+        assert resp.json()["huaweiFound"] is False
+
+
 class TestDiscoverReportsDisabledSensors:
     """POST /api/setup/discover must tell the wizard which sensors are
     unmapped because their entity is disabled in HA (#549).
@@ -1123,6 +1218,7 @@ def _run_discover(self, ctrl, platform_sensors, platform_disabled, platform):
             {
                 "growatt_found": False,
                 "growatt_device_id": None,
+                "huawei_found": False,
                 "solax_found": True,
                 "nordpool_found": False,
                 "nordpool_area": None,
@@ -1243,6 +1339,7 @@ def _integrations(self, **overrides) -> dict:
         base = {
             "growatt_found": False,
             "growatt_device_id": None,
+            "huawei_found": False,
             "solax_found": False,
             "nordpool_found": False,
             "nordpool_area": None,

docs/SOFTWARE_DESIGN.md

@@ -494,12 +494,21 @@ On first startup with no sensors configured, or when the user triggers discovery
 
 The HA WebSocket API (`config/entity_registry/list`) returns every registered entity with its `platform` field.
 
+Matching is by exact platform name, so a supported inverter reached through a
+different integration is not detected — Huawei LUNA2000/EMMA via
+`huawei_emma_management` rather than the stock `huawei_solar`, for example.
+Detection therefore narrows the wizard's **defaults**, never its **choices**:
+every platform stays selectable so such a user can pick theirs and map the
+sensors by hand (#621).
+
 Detected integrations:
 
 | Category  |   HA Platform       | Detected As |
 |-----------|---------------------|-------------|
 | Inverter  | `growatt_server`    | Growatt     |
 | Inverter  | `solax_modbus`      | SolaX       |
+| Inverter  | `solis_modbus`      | Solis       |
+| Inverter  | `huawei_solar`      | Huawei      |
 | Price     | `nordpool`          | Nordpool    |
 | Price     | `octopus_energy`    | Octopus Energy |
 | Forecast  | `solcast_solar`     | Solcast solar forecast |
@@ -597,7 +606,7 @@ The setup wizard is a 6-step flow for first-time configuration. It is triggered
 #### Wizard Steps (Frontend: `SetupWizardPage.tsx`)
 
 1. **Scan** — Calls `/api/setup/discover` to auto-detect integrations and sensors
-2. **Review Sensors** — Displays discovered sensor mappings, allows manual correction, selects inverter platform. Blocked while a required sensor is unmapped, or while a required sensor's only entity is disabled in HA (the entities to enable are listed by name)
+2. **Review Sensors** — Displays discovered sensor mappings, allows manual correction, selects inverter platform. Every platform is selectable regardless of what was detected; the per-platform status dot reports detection, and the auto-detected platform is merely preselected. Blocked while a required sensor is unmapped, or while a required sensor's only entity is disabled in HA (the entities to enable are listed by name)
 3. **Electricity Pricing** — Configure price area, provider (Nordpool/Octopus), markup, VAT (pre-filled from discovery hints). Blocked until the selected provider's required configuration is filled in, mirroring the server-side check on `/api/setup/complete`
 4. **Battery** — Set capacity, SOC limits, power rating, cycle cost
 5. **Home** — Set consumption, fuse current, voltage, phase count (pre-filled from detected phase count)