Add experience-level and remote-compute onboarding preferences

[bdraco]

What does this implement/fix?

Adds two onboarding preferences, an experience level (beginner, ui, yaml) and a remote-compute-only flag, and makes the onboarding steps aware of them. The use-case question only appears outside the Home Assistant add-on, and the Wi-Fi step is skipped when the install is remote-compute-only. Existing users (YAML configs on disk, or a previously completed onboarding) are silently defaulted to the "Prefer YAML" experience and never see the wizard, so they aren't re-prompted.

The dashboard reads these on every connect, so preferences now ride the subscribe_events initial_state snapshot rather than a separate config/get_preferences round-trip. To serve that snapshot synchronously without blocking the event loop, preferences moved to a RAM-canonical PreferencesStore (the same per-file debounced Store as the device-metadata and peer-link settings): loaded once at startup, migrated out of the shared .device-builder.json sidecar into their own .device-builder-preferences.json on first run, mutated in RAM with a debounced write. That storage conversion also closes the read-vs-write race the old sidecar read-modify-write had, where two concurrent set_preferences writes could read the same baseline and clobber each other's field.
Related issue or feature (if applicable):

• N/A

Types of changes

• Bugfix (non-breaking change which fixes an issue) — bugfix
• New feature (non-breaking change which adds functionality) — new-feature
• Enhancement to an existing feature — enhancement
• Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
• Refactor (no behaviour change) — refactor
• Documentation only — docs
• Maintenance / chore — maintenance
• CI / workflow change — ci
• Dependencies bump — dependencies

Frontend coordination

• No frontend change needed
• Companion frontend PR: Add experience-level onboarding wizard and remote-compute mode device-builder-frontend#818

Checklist

• The code change is tested and works locally.
• Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
• Tests have been added or updated under tests/ where applicable.
• components.index.json / definitions/components/*.json have not been hand-edited (regenerate via script/sync_components.py if a sync is needed).
• Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

[codspeed-hq]

Merging this PR will not alter performance

✅ 27 untouched benchmarks

---

_{Comparing onboarding-experience-levels (56a0c82) with main (d6054e3)}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.50%. Comparing base (d6054e3) to head (56a0c82).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1445   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files         222      222           
  Lines       17253    17305   +52     
=======================================
+ Hits        17168    17220   +52     
  Misses         85       85           

Flag	Coverage Δ
py3.12	99.47% <100.00%> (+<0.01%)	⬆️
py3.14	99.50% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
esphome_device_builder/controllers/onboarding.py	100.00% <100.00%> (ø)
esphome_device_builder/device_builder.py	97.76% <100.00%> (+<0.01%)	⬆️
esphome_device_builder/models/onboarding.py	100.00% <100.00%> (ø)
esphome_device_builder/models/preferences.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Blocking issues found — see the review comment above.

[Copilot]

Pull request overview

Adds two new onboarding-related user preferences (experience level and remote-compute-only) and updates the onboarding flow to conditionally include/skip steps based on environment (HA add-on vs non-HA) and preferences. It also migrates pre-existing installs to the YAML experience at startup so existing users aren’t re-prompted by the new onboarding version.

Changes:

• Introduce ExperienceLevel and remote_compute_only in UserPreferences, and bump onboarding flow version to 2.
• Make onboarding/get_state return an environment-/preference-aware step list (use-case + experience steps, and optional Wi-Fi step).
• Add startup migration to default pre-existing installs to the YAML experience and mark onboarding acknowledged; expand tests and API docs accordingly.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/test_onboarding_controller.py	Expands coverage for the new onboarding step list and migration behavior.
esphome_device_builder/models/preferences.py	Adds ExperienceLevel enum and new preference fields (experience_level, remote_compute_only).
esphome_device_builder/models/onboarding.py	Adds new step IDs and bumps ONBOARDING_VERSION to 2.
esphome_device_builder/device_builder.py	Runs onboarding migration during startup before controllers begin serving requests.
esphome_device_builder/controllers/onboarding.py	Computes step list based on on_ha_addon + preferences; implements migration for pre-existing installs.
docs/API.md	Documents new preference fields and the updated onboarding step semantics.

[bdraco]

Addressed the review in aeb219f.

• Migration config detection now uses esphome.util.list_yaml_files, so .yml configs count too and the extension, secrets, and dotfile rules stay in sync with the device scanner.
• The scan OSError is now logged instead of silently swallowed, so a read failure during startup migration is visible rather than quietly reclassifying a real install as fresh.
• Added a .yml migration test and an unreadable-dir test; onboarding.py is back to 100% patch coverage.

Left the use_case step status tracking experience_level as is; it is intentional and documented inline, since the wizard always answers both in the same pass.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

No blocking issues found.

[bdraco]

Addressed the silent-failure note in 1800546. _has_device_configs now fails safe by direction: a missing config dir stays False (a genuinely fresh install), but a dir that exists and cannot be read returns True, so a transient read error never reclassifies a real install as fresh or re-pops the wizard. FileNotFoundError keeps the fresh-install path; other OSError assumes pre-existing and logs. Added a mocked PermissionError test; onboarding.py stays at 100% coverage.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

No blocking issues found.

[bdraco]

Addressed the two style nits in e720453: the ExperienceLevel multi-line docstring summary now starts on the line after the opening quotes, and the _step test helper is typed OnboardingStepStatus | None to match its next(..., None) behaviour.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

No blocking issues found.

[bdraco]

Added user preferences to the subscribe_events initial_state snapshot (46c93a8). The frontend gates first-paint UI (experience level, remote-compute creation hiding) on these, so shipping them in the snapshot lets the dashboard paint without a separate config/get_preferences round-trip whose failure would otherwise uniquely break device creation. Sync read in _send_initial per the hot-path rule; covered by a new initial_state test and noted in docs/API.md. Pairs with frontend PR esphome/device-builder-frontend#818.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

No blocking issues found.

[esphbot]

No blocking issues found.

[bdraco]

The three silent-failure items are the same data-loss concern (a corrupt/incompatible blob silently defaulted and the source destroyed) — fixed in 7d50de9 by preserving undecodable data rather than erasing it:

• _decode is now strict and propagates a corrupt/non-object/undecodable payload instead of swallowing it into defaults.
• PreferencesStore.async_load catches that and renames the corrupt dedicated file to .device-builder-preferences.json.corrupt before falling back to defaults, so the next debounced save can't overwrite a recoverable read (silent-failure Fix be when i want the catalog for a specific device #2).
• _migrate_read_shared_sync decodes the sidecar _preferences blob directly and, on failure, logs and returns None so the migration leaves the blob in the sidecar and does not strip it — recoverable legacy data is never discarded (silent-failure Add section config API and user preferences #1 + Handle hint and errors in yaml #3, which converge: distinguish "key absent" from "decode failed" and abort the strip).

Both paths _LOGGER.exception(...) so the failure is observable. The now-unused _prefs_from_data is gone. Tests updated to assert the .corrupt preservation and the left-in-place sidecar blob; new modules stay at 100% coverage; full suite green (4924 passed).

Re the daemon-lifecycle checklist item: confirmed present in device_builder.py — await self.config.async_load() runs in start() before the onboarding migration and before any client can subscribe (L341), and await self.config.stop() runs in stop() (L480). Also verified live earlier: a startup migrates the sidecar blob into the dedicated file, a set_preferences toggle persists, and the value survives a restart.

[esphbot]

@bdraco: The three silent-failure items are the same data-loss concern (a corrupt/incompatible blob silently ...

Solid fix. One residual gap. async_save_now swallows write errors — _async_handle_write catches and logs, storage.py:209-215, never re-raises. So await async_save_now() returning ≠ dedicated file persisted. Write fails on disk-full or EACCES. _migrate_strip_shared_sync strips sidecar anyway. Same data loss this rework targets. Gate strip on confirmed persistence. Check self._store.path.exists() before strip, or re-load and assert non-None.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Blocking issues found — see the review comment above.

[bdraco]

Good catch — suppress(OSError) defeated the recovery guarantee silently. Fixed in 279b72c: _preserve_corrupt_file now logs the failure (_LOGGER.warning(..., exc_info=True)) instead of swallowing it, so a failed preservation leaves a trace. Dropped the now-unused suppress import and added a test that forces the rename to raise and asserts the warning is logged.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Blocking issues found — see the review comment above.

[bdraco]

Fixed in 4c09b8c.

Important #1 (strip after unconfirmed write — real data-loss path): confirmed against helpers/storage.py:209-215 — _async_handle_write catches Exception and only logs, so async_save_now() returns even on a failed atomic write. Migration now gates the strip on a confirmed dedicated-file write (if not self._store.path.exists()): an unconfirmed flush leaves _preferences in the sidecar and logs a warning, so the next start retries instead of losing prefs. (The pre-state is known-absent — we only migrate when async_load() returned None — so path.exists() is a sound proxy for "the atomic move landed.")

Silent Failure #2 (corrupt dedicated file skips sidecar recovery): folded in. Both the normal first-run path and the corrupt-file path now route through one _migrate_from_sidecar helper, so a corrupt dedicated file is renamed aside and then the legacy sidecar is still tried before defaulting. Verified end-to-end on a live backend: corrupt dedicated file + valid legacy _preferences blob → .corrupt preserved, fresh dedicated file written from the recovered values, sidecar key stripped, _labels kept.

Tests: added test_async_load_keeps_sidecar_when_migration_write_unconfirmed (monkeypatches atomic_write to raise → asserts the sidecar key survives and the warning logs) and test_async_load_corrupt_dedicated_file_recovers_from_sidecar.

Silent Failure (acknowledged-before-durable write): intended. This is the RAM-canonical Store contract shared with the peer-link and device-metadata stores — set_preferences returns after the RAM mutation + debounced save, durability rides the shutdown flush, and the background write already logs failures (storage.py:215). Surfacing per-write durability to the WS caller would mean abandoning the debounce that the whole pattern exists for, so leaving it as the documented tradeoff.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

No blocking issues found.

[bdraco]

Fixed in 762a147 — update() now returns self._copy().

One correction worth flagging: mutate() had the same latent footgun, not the copy-returning behaviour the review assumed. It did self._state = result; return result, so the returned object was the live canonical reference too. Fixed both, so all three accessors (snapshot/update/mutate) now uniformly return an independent copy and the "a caller can't corrupt canonical RAM" invariant holds across the whole surface.

Added test_mutators_return_independent_copies, which mutates each return value in place and asserts the next snapshot() is unchanged. Both current callers were already safe (set_prefs only serializes the result; both onboarding mutate callers ignore it), so no behaviour change — just the invariant closed.

[bdraco]

Both items addressed (this review was at 4c09b8c; HEAD is now 97c956a).

Suggestion #1 (update() returns canonical ref): fixed in 762a147. mutate() had the same latent issue (it returned result, i.e. self._state, not a copy), so I fixed both — all three of snapshot/update/mutate now return self._copy().

MEDIUM (failed preserve can cascade into destroying the corrupt file): real gap in the "preserved, never destroyed" guarantee — fixed in 97c956a. When _preserve_corrupt_file's rename fails, the store now sets _persist_disabled, which suppresses every write (update/mutate route through one _schedule_save choke point, and migration early-returns after recovering into RAM). The session still serves recovered prefs from RAM, but nothing can overwrite the corrupt file; the next restart retries the preserve.

Test test_failed_preserve_disables_writes_to_protect_corrupt_file makes only the .corrupt rename fail (so a subsequent real atomic_write would otherwise succeed and clobber it) and asserts the corrupt bytes survive both load and a later update() + flush.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

No blocking issues found.

[bdraco]

Suggestion #1 (-O-strippable assert): fixed in 2f25f43. Swapped the assert self.config is not None for if self.config is not None:, matching the if self.devices: / if self.remote_build_offloader is not None: guards in the same block — -O-safe and uniform. The guard always passes (config is created in start() before subscribe is served, as the comment notes), so preferences stays always-present in practice.

MEDIUM (fire-and-forget persistence): keeping as the documented RAM-canonical design, as the review notes is acceptable — same contract as the peer-link and device-metadata stores; durability rides the shutdown flush and the background write already logs failures (storage.py:215). Surfacing per-write durability to the WS caller would mean abandoning the debounce the pattern exists for. If we later want a signal, the right shape is a store-health event rather than a synchronous flush-confirm on the hot write path; out of scope here.

[esphbot]

PR Review — Add experience-level and remote-compute onboarding preferences

Solid, well-defended feature — merge-ready. One minor, pre-existing input-validation nuance worth a glance.

Strengths (verified against the branch tip 2f25f43, not just the diff):

• The RAM-canonical PreferencesStore migration is genuinely careful: the sidecar strip is gated on a confirmed dedicated-file write, corrupt files are renamed .corrupt (and writes disabled if even that fails), and an undecodable legacy blob is left in place — every "preserved, never destroyed" claim holds in the code.

• Migration is correctly idempotent and self-healing: a SIGKILL before the debounced write flushes just re-runs migrate_preexisting_install on next boot, since experience_level is None re-triggers it.

• _has_device_configs routes through esphome.util.list_yaml_files (so .yaml/.yml/secrets/dotfiles stay in lockstep with the device scanner) and fails safe by direction — missing dir = fresh, unreadable dir = pre-existing.

• snapshot/update/mutate uniformly return independent copies, so the canonical RAM state can't be corrupted by a borrowed reference.

• The prior review rounds were thorough and bdraco resolved every substantive item (data-loss on migration, corrupt-file recovery, copy semantics, -O-strippable assert, .yml coverage).

• Minor (suggestion Add section config API and user preferences #1): config/set_preferences raises InvalidFieldValue → INTERNAL_ERROR on a bad experience_level, and silently coerces a non-bool remote_compute_only to True. Pre-existing mashumaro behaviour, widened by the new fields; frontend is the only caller and sends valid values.

---

🟢 Suggestions

1. set_preferences surfaces InvalidFieldValue / silently coerces non-bool fields (`esphome_device_builder/controllers/config/controller.py`, L118-122)

set_prefs passes the raw kwarg dict straight into UserPreferences.from_dict (via prefs.update). I confirmed two mashumaro edge behaviours against a mirror of the new model:

• An invalid enum value (experience_level: "bogus") raises mashumaro.exceptions.InvalidFieldValue, which is uncaught here and reaches the client as INTERNAL_ERROR rather than a clean INVALID_ARGS.
• remote_compute_only: "false" (a string) does not raise — any non-empty string coerces to True. Since this flag hides device-creation entry points and skips the Wi-Fi step, a malformed client value silently flips a load-bearing UI gate.

Why it's only a suggestion: the frontend is the sole caller and the onboarding flow sends well-typed JSON (true/false, valid enum strings), so this isn't hit in practice. It's also pre-existing behaviour for the existing enum fields (theme, dashboard_view), only widened by the two new fields — not a regression this PR introduces.

If you want to harden the boundary, validating/coercing the known fields in set_prefs before update would turn both cases into a clean CommandError(INVALID_ARGS, ...), matching the careful validation already done in onboarding/set_wifi_credentials. Out of scope is also a fine call.

update_fields = {k: v for k, v in kwargs.items() if k not in ("client", "message_id")}
return self.prefs.update(update_fields)

---

Checklist

• Migration preserves data on write failure (gated on confirmed write)
• Corrupt-file recovery preserves data; cascade-destroy guarded
• RAM-canonical copy semantics (snapshot/update/mutate)
• Backward-compat migration for existing installs (.yaml + .yml)
• Snapshot reads sync, no executor hop on subscribe hot path
• Input validation at WS boundary (set_preferences) — suggestion ＃1
• Shutdown flush wired (config.stop drains store callbacks)
• API/docs updated

---

Silent Failure Analysis

🟡 **MEDIUM** — silent persistence drop with success return (`esphome_device_builder/controllers/config/_preferences_store.py:138-142`)

Risk: After a corrupt-file rename fails and _persist_disabled is set, update()/mutate() still mutate RAM and return the new UserPreferences to the client as if saved, so config/set_preferences reports success while the change silently never reaches disk and is lost on restart.

def _schedule_save(self, *, delay: float) -> None:
    if self._persist_disabled:
        return
    self._store.async_delay_save(self._snapshot, delay=delay)

Fix: Surface the persistence-disabled state to callers (e.g. raise or flag a persisted=false in the response) instead of returning the new prefs as an unqualified success.

---

Automated review by Kōan (Claude) HEAD=2f25f43 4 min 49s

[esphbot]

No blocking issues found.

[bdraco]

Both deferred as out-of-scope for this PR.

Suggestion #1 (set_preferences input validation): pre-existing mashumaro behaviour for the existing enum/bool fields (theme, dashboard_view), only widened by the two new fields, not introduced here. The frontend is the sole caller and the onboarding flow sends well-typed JSON (true/false, valid enum strings), so neither the InvalidFieldValue → INTERNAL_ERROR path nor the non-bool coercion is reachable in practice. Hardening the WS boundary is a real improvement but it's a separate concern that should cover all known fields uniformly (not just the two this PR adds) and changes the command's error contract — better as its own focused PR than bolted onto this one.

MEDIUM (silent persistence drop when _persist_disabled): keeping as-is. _persist_disabled is only set on a double disk failure — an undecodable dedicated file and a failed rename-aside — at which point the disk is effectively unwritable and the disable is already logged at WARNING. In that state the change is still applied in RAM for the session; it's just not durable, the same contract as the fire-and-forget write path. Threading a persisted=false through the response (and the frontend) to signal a state that needs a corrupt file plus a broken rename to occur isn't worth the surface; if we ever want this, a store-health event is the right shape, not a per-write flag on the hot path.