fix: let the setup wizard offer platforms auto-detection missed - #634
Conversation
A user running Huawei LUNA2000/EMMA through the huawei_emma_management
integration cannot complete setup at all. Two defects stack.
First, /api/setup/discover never sent huawei_found. discover_integrations()
produces it, but the endpoint forwarded growatt/solax/solis and dropped it,
so discovery.huaweiFound was undefined for every user and the Huawei tab's
`disabled={wizardMode && !huaweiDetected}` was unconditionally true. The tab
was not disabled on failed detection — it was never enabled. This stayed
hidden because a stock huawei_solar install is preselected via
detectedInverterPlatforms and so never needs to click the trigger, and
because the e2e fixture hand-wrote a huaweiFound field the API did not
return.
Second, the disable was never Huawei-specific: all four platform tabs and
the three SolaX variant pills gated on detection identically, so an
unrecognised Solis build was locked out the same way. Detection matches a
fixed list of HA platform names, so any supported inverter reached through
a custom integration fails it. Detection should narrow the defaults, not
the choices — the status dot already reports what was found.
Third, handleScan is a useCallback with an empty dep list and read
inverterForm.inverterPlatform from a stale closure. With nothing detected
it reset sensors.platform to the default while the form kept the user's
choice; allRequiredFilled resolves sensors via sensors.platform but takes
its required-key list from the selected tab, so once those diverged the
sensor step could never be completed. That trap is sprung by Re-scan, which
is exactly what a user with failed detection reaches for.
Closes #621
|
@claude-bot review |
There was a problem hiding this comment.
Review: fix/issue-621-wizard-platform-gate (closes #621)
Root-cause match: yes, all three defects verified against source.
- Defect A —
backend/api.py:2963now sends"huawei_found": integrations["huawei_found"]. Confirmedhuawei_foundis unconditionally initialized indiscover_integrations()'s base dict (core/bess/ha_api_controller.py:3239-3243), so direct indexing (not.get()) is correct and matches the existinggrowatt_found/solax_foundpattern — not a new convention. - Defect B — every
disabled={wizardMode && !xDetected}gate is gone. I diffed the PR branch'sSensorConfigSection.tsxfor the stringdisabledand the only remaining hits are the unrelateddisabledSensorsfield/docstring (#549 feature). All four tab triggers and the three SolaX variant pills (solax_modbus_native/_growatt_min/_growatt_sph) are unconditionally selectable now. ThegrowattDetected/cloudDetected/modbusDetected/solisDetected/huaweiDetectedlocals are still consumed by the status-dotclassName, so removingdisableddidn't leave dead/unused bindings. - Defect C —
selectedPlatformRef(SetupWizardPage.tsx) mirrorsinverterForm.inverterPlatformevery render;handleScan's empty-depuseCallbackreadsselectedPlatformRef.currentinstead of the stale closure value. This is the same ref-mirroring shape already used in the same file forexistingSensorsRefandwizardNeededRef— reuses an existing pattern rather than inventing a new one.
Would the tests catch a regression? Yes, and the PR earns that claim rather than asserting it:
- Backend:
TestDiscoverForwardsInverterDetectionFlagsasserts the endpoint payload (notdiscover_integrations()directly, which already had passing coverage and is exactly why the bug survived — good instinct catching that distinction). - Frontend e2e: two new Playwright specs assert user-visible outcomes (tab
toBeEnabled(), panel visibility, Next-button enabled after manual entry) rather than the removeddisabledprop itself. I checked every selector used (Not detected — enter entity IDplaceholder,Huawei battery device IDplaceholder,LUNA2000text,Re-scanbutton,Next: Electricity Pricingbutton) against the actual markup in both changed components — all match. - PR body documents an actual RED→GREEN cycle for each of the three defects independently (including reverting only the ref fix while keeping the tab fix, to isolate defect C) — satisfies the "must be seen to fail without its fix" rule.
Minimal / no scope creep: yes. Backend: 1 line. Frontend: prop removals + one ref. The ~9 existing test fixtures that gained "huawei_found": False, are a forced, mechanical consequence of switching to direct dict indexing (would otherwise KeyError), not unrelated churn. docs/SOFTWARE_DESIGN.md's added solis_modbus→Solis / huawei_solar→Huawei rows match _INVERTER_PLATFORMS at core/bess/ha_api_controller.py:3758-3763 exactly, and the doc's "defaults, not choices" framing matches the shipped behavior.
Rule compliance: no Optional[x], no hasattr/getattr-with-default, no new classes, camelCase conversion untouched (goes through the existing convert_keys_to_camel_case call), no sensor access bypassing ha_api_controller. PR body explicitly states the fix is scoped as a local bug fix (forward a dropped field, remove a gate that was never meant to block selection, fix a stale-closure read) rather than a new responsibility bolted onto an existing method — satisfies the scope-assessment checklist item.
Nits (non-blocking):
- I could not execute
black/ruff/tsc/eslintmyself in this review environment (no toolchain available), so I'm relying on the PR's stated localquality-check.shrun rather than independently reproducing it. Nothing in the diff reads as a formatting or type issue on inspection.
No blockers found. Approving.
…ove (#653) * fix: stop the fleet prune from destroying the worktrees it cannot remove `git worktree remove` is sandbox-denied, and unlike `git worktree add` it fails DESTRUCTIVELY. Removal deletes the working tree first and only then unlinks `.git/worktrees/<name>` -- and that unlink is the denied one: error: failed to delete '.../worktrees/backlogger': Operation not permitted error: failed to delete '.git/worktrees/backlogger': Operation not permitted By then ~393 tracked files are gone. It does not roll back. What is left is a carcass: a registered worktree whose `git status` is a few hundred ` D` lines and nothing else. Both prune loops read that as "uncommitted tracked changes" and correctly refuse to auto-delete it -- so the failure makes the worktree permanently unprunable BY ITSELF. Re-running hits the no-`--force` refusal; `--force` re-hits the denial. `git worktree prune` performs the same unlink, so it cannot clear the wreckage either. 13 carcasses accumulated across three sweeps (#568, #596, #597, #600, #601, #603, #609, #612, #617, #629, #633, #634, #641) before anyone read the diff. The last sweep reported them back as "a real backlog of stranded edits worth reviewing" -- they were its own wreckage from the previous runs, and not one byte of real work was in them. Because the filename set is identical in every worktree, so is APFS's readdir order, so every carcass loses the SAME ~393 paths (`core/`, `frontend/`, `bess_manager/`, `pyproject.toml`, ...). Identical damage across many worktrees is the signature, not a coincidence. - Both prune loops now report `PRUNE` and emit one `!`-prefixed command for the maintainer to run unsandboxed, instead of removing anything themselves. - Both classify a dirty set that is entirely ` D` as `CARCASS`, distinct from real edits. The predicate anchors on `^ D ` (unstaged deletions only), so a STAGED deletion still reads as intentional work. - `local-agent-environment.md` gains the `remove`/`prune` half of the `.git/worktrees` denial, next to the `add` half it already documented. `verify-sandbox.sh:88` predicted this in a parenthetical ("a stray directory when `git worktree remove` then fails"); nothing acted on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FQA38o22RkHZJ4zEHDZS3 * fix: measure the prune half, which exits 0 while failing The previous commit inferred that `git worktree prune` was denied because it performs the same `.git/worktrees/<name>` unlink as `remove`. Measured now, using a carcass as the probe -- `rm -rf` its directory, then prune: $ git worktree prune -v; echo "exit=$?" Removing worktrees/backlogger: gitdir file points to non-existent location error: failed to delete '.../.git/worktrees/backlogger': Operation not permitted exit=0 Denied as expected, but it EXITS 0. `remove` at least exits 255; prune reports success having done nothing, so `git worktree prune && echo done` prints done. That is the nastier of the two and was not in the inferred version. Two further measured facts change the guidance: - The denial is on the `.git/worktrees/<name>` unlink ONLY. `rm -rf` of the working tree from Bash SUCCEEDS. An agent can therefore always destroy the files and never the registration, and half-doing it converts a carcass into a `prunable` phantom -- no better, still needing the same unsandboxed fix. - A phantom breaks both classification loops. Its directory is gone, so `git -C "$wt" branch --show-current` fails, `$b` comes back empty, and the detached-HEAD guard swallows it as "SKIP (detached)". Both loops now test `[ -d "$wt" ]` first and report PHANTOM. Verified over four cases: missing dir with and without a branch name, detached, and on a branch. The emitted cleanup command gains a trailing `git worktree prune` (a phantom has no directory, so `remove` has nothing to work with), uses `[ -n "$b" ]` instead of `&&` since a phantom yields no branch name, and now says it must run from a NON-worktree-isolated session -- an isolated one refuses the `cd` to the shared checkout, which is why the first cleanup attempt silently did nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FQA38o22RkHZJ4zEHDZS3 * docs: record that `git branch -D` warns on .git/config but strands nothing Third instance of the `.git/config` denial, alongside `checkout -b --track` and `push -u`, and the same exit-0-while-partially-failing shape as the prune bullet: $ git branch -D worktree-backlogger; echo "exit=$?" error: could not lock config file .../.git/config warning: update of config-file failed Deleted branch worktree-backlogger (was 6c70a77). exit=0 The first draft of this bullet asserted the denial leaves a stale `[branch "<name>"]` stanza behind. Checked before committing, and it does not: grepping .git/config afterwards found no stanza for the deleted branch, and the only stale one in the file is an unrelated `undefined`. The branch had no stanza to drop in the first place -- writing one needs `push -u` or `checkout -b --track`, both denied by the two bullets above, so branches created under this sandbox never have one. Recorded as the general case, with the pre-sandbox branch explicitly marked untested rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FQA38o22RkHZJ4zEHDZS3 * docs: reconcile worktree-remove bullets with the rest of the permissions doc The new destructive-failure bullets for `git worktree remove`/`prune` contradicted three claims in the same file: the unattended list (which still named `git worktree remove`), the "sandbox makes the unattended list safe" thesis, and the "git already refuses the dangerous case" reasoning for the ask list. A reader of only the Permissions section could conclude the command was safe to call from sandboxed Bash — the exact bug the skills no longer call. Carve the two verbs out as explicit exceptions and scope the git-refuses reasoning to the cases git actually covers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: report worktree paths, not branch names, in the prune output The sweep's emitted maintainer command reconstructed `.claude/worktrees/<name>` from branch names, but `git worktree list` yields paths and includes sibling worktrees outside `.claude/worktrees/` — the branch name alone cannot locate the worktree, so removal would silently miss its target. Report `$wt` in the PRUNE/CARCASS lines and drive the command from those paths. Also count PHANTOM in implement-issue's emit-one-command line, matching sweep-prs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: fold the branch delete into Step 11's deferred worktree removal The After-Merge cleanup splits in two: `ExitWorktree action=remove` clears the worktree in-session, or removal is handed to the maintainer when the session has already left. The old item 3 ran `git branch -D` right after either path, but git refuses to force-delete a branch while its worktree registration persists — the exact state the deferred path leaves behind, since only the maintainer's not-yet-run command clears the registration. Emit the branch delete as part of that same deferred command (remove first, then delete, as sweep-prs does), and scope item 3 to the in-session path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
/api/setup/discovernow sendshuawei_found, which it silently droppedRoot cause
Three defects stack for a user running Huawei LUNA2000/EMMA through the
huawei_emma_managementintegration (reported by @valexi7).A — the flag was never sent.
backend/api.py:2963built the discover payload withgrowatt_found,solax_found,solis_foundandhuawei_device_id, but nohuawei_found. The value exists upstream (core/bess/ha_api_controller.py:3283) and was simply dropped;grep -rn huawei_found backend/returned nothing. Sodiscovery.huaweiFoundwasundefinedfor every user anddisabled={wizardMode && !huaweiDetected}was unconditionally true. The Huawei tab was not "disabled when detection fails" — it was never enabled.It stayed hidden because
detectedInverterPlatformsis populated, so a stockhuawei_solaruser gets the Huawei panel preselected and never clicks the trigger. Only the undetected user needs the click. The e2e fixture hand-wrote ahuaweiFound: falsefield the real API did not return, so no test could catch it.B — the gate was never Huawei-specific. All four tabs (
:360,:372,:384,:396) and the three SolaX pills (:458) gated on detection identically, so an unrecognised Solis build was locked out the same way. Detection matches a fixed list of HA platform names (_INVERTER_PLATFORMS), so any supported inverter reached through a custom integration fails it — the reporter's entities carryhuawei_emma_management, nothuawei_solar, which is also why theirhuawei_device_idand sensor maps came back null.C — Re-scan reverted the choice.
handleScanis auseCallback(..., [])and readinverterForm.inverterPlatformfrom a stale closure. With nothing detected it resetsensors.platformto the default while the form kept the user's choice.allRequiredFilledresolves sensors viasensors.platformbut takes its required-key list from the selected tab, so once those diverged the sensor step could never be completed no matter what was typed — and Re-scan is exactly what a user with failed detection reaches for.Fix
Detection narrows the defaults, never the choices; the per-platform status dot already reports what was found. So: forward the missing flag, drop the
disabledprops, and read the selected platform through a ref instead of a stale closure. Manual completion already worked — the device-ID input, service-domain field and free-text sensor entries all persist without any detection dependency.Backend discovery is deliberately not changed:
huawei_found: falseis correct for this user. The bug is the gate, not the detection.Test plan
./scripts/quality-check.shpasses locally — 2055 passed, vitest 125 passed, tsc + eslint cleanpytest -m slow— 548 passed, 7 skipped/code-review— no CONFIRMED findingsPOST /api/setup/discoveragainst mock-HA: undetected scenario returnshuaweiFound: false;ci-wizard-huawei-luna2000returnshuaweiFound: true, huaweiDeviceId: 'dev-huawei-battery-001'. The key was absent entirely before this change, so a correctly detected Huawei user also got a grey dot.The one failure is
redirects to /setup when no sensors are configured, and it is pre-existing — it fails identically with this entire frontend diff reverted and rebuilt, on a clean stack with a pristine fixture. Locallyci-wizard-settings.jsonyieldswizardNeeded: false, so the redirect cannot fire. Worth investigating separately; it may not be proving anything in CI either.Evidence the test discriminates
Two independent mutations, one per defect:
Defect B — before the fix,
every platform tab stays selectable when nothing is detectedfailed with Playwright resolving the element as:<button disabled role="tab" ... id="radix-«r0»-trigger-huawei">,element is not enabledDefect C — with the tab fix in place but only
selectedPlatformRefreverted toinverterForm.inverterPlatform, rebuilt and re-run:a manually selected platform survives a re-scanFAILED —expect(locator).toBeEnabled() failed / Received: disabledon the Next buttonDefect A —
TestDiscoverForwardsInverterDetectionFlagsfailed RED withKeyError: 'huaweiFound'(3 tests), green after adding the key.Outcome-level coverage
TestDiscoverForwardsInverterDetectionFlagsasserts the endpoint payload, notdiscover_integrations().test_scenario_discovery.py:296already asserted the controller and passed throughout — that is precisely why the gap survived, so the new tests pin the contract at the layer that was broken.disabledprop or any internal flag.docs/SOFTWARE_DESIGN.mdStage 1 detection table was missingsolis_modbusandhuawei_solar, and the wizard step description implied selection follows detection. Both corrected, since this change makes those claims wrong.Closes #621