Stop duplicating Wi-Fi secrets and attribute secrets.yaml parse errors to the user (#2624)

Author: bdraco

docs/API.md

@@ -120,7 +120,7 @@ Connections that arrive on the trusted ingress site (HA add-on supervisor proxy)
 |---------|------|----------|-------------|
 | `devices/list` | — | `DevicesResponse` | List configured + importable devices |
 | `devices/get_states` | — | `dict` | Get device online/offline states |
-| `devices/create` | `{name, friendly_name?, board_id?, ssid?, psk?, file_content?, overwrite?}` | `WizardResponse` | Create device. With `friendly_name`, `name` is the hostname — validated (lowercase letters, digits, hyphens, underscores; no leading/trailing hyphen; at most 31 chars; `INVALID_ARGS` otherwise) and used verbatim, never rewritten — and the cleaned `friendly_name` becomes `esphome.friendly_name:`. Without it (or when it cleans to empty), `name` is the user's raw display label — capitalisation, inter-word spaces, and unicode are preserved; surrounding whitespace is trimmed. The backend slugifies the cleaned value for `esphome.name:` and the YAML filename and writes the cleaned original into `esphome.friendly_name:`. Callers that already pass a slugified value get the same hostname/friendly_name pair as before (slug-of-slug is a no-op). Three flows: `file_content` writes the supplied YAML as-is; `board_id` generates from a board template; with neither, emits a minimal esp32 stub for the "empty configuration" path. **Wi-Fi handling:** a supplied `ssid` / `psk` is written to `secrets.yaml` (validated, shared with `config/set_wifi_credentials`) and the generated YAML references `!secret wifi_ssid` / `!secret wifi_password` — bare credentials are never written into the device YAML, and the next device reuses the shared secret. With no `ssid`, the generator emits `!secret` when Wi-Fi secrets already exist, else (for a board with no other network) a no-network stub. A board offering onboard-ethernet suggested hardware is **wired by default** — its `ethernet:` block is auto-pulled and the `wifi:` block dropped (see the network-providers note under [Boards](#boards)). A filename collision returns `ALREADY_EXISTS`; pass `overwrite: true` to replace the YAML in place, preserving the existing device's metadata (labels / comment / board_id) and StorageJSON. For a package board, `WizardResponse.warning` is set when the config was kept despite a validation failure confined to remote package resolution (same contract as `devices/import`). |
+| `devices/create` | `{name, friendly_name?, board_id?, ssid?, psk?, file_content?, overwrite?}` | `WizardResponse` | Create device. With `friendly_name`, `name` is the hostname — validated (lowercase letters, digits, hyphens, underscores; no leading/trailing hyphen; at most 31 chars; `INVALID_ARGS` otherwise) and used verbatim, never rewritten — and the cleaned `friendly_name` becomes `esphome.friendly_name:`. Without it (or when it cleans to empty), `name` is the user's raw display label — capitalisation, inter-word spaces, and unicode are preserved; surrounding whitespace is trimmed. The backend slugifies the cleaned value for `esphome.name:` and the YAML filename and writes the cleaned original into `esphome.friendly_name:`. Callers that already pass a slugified value get the same hostname/friendly_name pair as before (slug-of-slug is a no-op). Three flows: `file_content` writes the supplied YAML as-is; `board_id` generates from a board template; with neither, emits a minimal esp32 stub for the "empty configuration" path. **Wi-Fi handling:** a supplied `ssid` / `psk` is written to `secrets.yaml` (validated, shared with `config/set_wifi_credentials`) and the generated YAML references `!secret wifi_ssid` / `!secret wifi_password` — bare credentials are never written into the device YAML, and the next device reuses the shared secret. With no `ssid`, the generator emits `!secret` when Wi-Fi secrets already exist, else (for a board with no other network) a no-network stub. A board offering onboard-ethernet suggested hardware is **wired by default** — its `ethernet:` block is auto-pulled and the `wifi:` block dropped (see the network-providers note under [Boards](#boards)). A filename collision returns `ALREADY_EXISTS`; pass `overwrite: true` to replace the YAML in place, preserving the existing device's metadata (labels / comment / board_id) and StorageJSON. For a package board, `WizardResponse.warning` is set when the config was kept despite a validation failure confined to remote package resolution (same contract as `devices/import`). A pre-write validation failure rooted in the config dir's `secrets.yaml` (a duplicate key, a parse error) is `INVALID_ARGS` phrased `Can't <action>: secrets.yaml has a duplicate key "<key>" (lines A and B). …` or `Can't <action>: secrets.yaml doesn't parse: …` (a rewrite that can't reach the live definition says `secrets.yaml defines "<key>" where the dashboard can't rewrite it`), never the generator-bug `INTERNAL_ERROR`; the same attribution applies to every mutation that validates (`clone`, `rename`, `import`, …), and the frontend keys its "Open secrets" action on that phrasing. |
 | `devices/import_bundle_token` | — | `{token}` | Mint a single-use token for the HTTP upload of one bundle. Bundles are uploaded over HTTP, not the WebSocket (see the note below); the response's `ImportBundleResponse` shape and the `overwrite` semantics are documented there. |
 | `devices/update` | `{configuration, friendly_name?, comment?, board_id?}` | `UpdateDeviceResponse` | Update device metadata (sidecar JSON) |
 | `devices/set_labels` | `{configuration, label_ids: string[]}` | `Device` | Replace this device's label assignments. Pass `[]` to clear. Unknown ids return `INVALID_ARGS`. Fires `device_updated` after the scanner reload when the assignments changed the row. |
@@ -344,8 +344,8 @@ Backed by [`controllers/editor.py`](../esphome_device_builder/controllers/editor
 | `config/get_preferences` | — | `UserPreferences` | Get user preferences |
 | `config/set_preferences` | `{theme?, dashboard_view?, experience_level?, remote_compute_only?, hide_device_builder?, version_history_enabled?, device_editor_layout?, secrets_editor_layout?, ...}` | `UserPreferences` | Update preferences (partial). `experience_level` is `beginner` / `expert` (or `null` until chosen); `remote_compute_only` marks an install as a remote build node; `hide_device_builder` (offered in the UI only while that is on) hides the dashboard's Device builder section entirely. `version_history_enabled` (default `true`) gates the git auto-commit of config edits; setting it `false` stops new commits and skips repo creation, leaving any existing repo intact. `device_editor_layout` is `visual` / `yaml` / `both` and `secrets_editor_layout` is `visual` / `yaml` (the secrets editor has no split view); they persist which editor panes the user keeps open. |
 | `config/get_secrets` | — | `[string]` | List secret key names |
-| `config/set_secret` | `{key, value, overwrite?}` | `{created}` | Atomically set one secret in secrets.yaml under a write lock; `overwrite=false` is create-if-absent |
-| `config/set_wifi_credentials` | `{ssid, password?}` | `{}` | Set `wifi_ssid` / `wifi_password` in `secrets.yaml` via a line-based rewrite that preserves standalone and inline trailing comments and other secrets. Backs the kebab "Set up Wi-Fi" action; the create wizard's own Wi-Fi entry is persisted by `devices/create`. Validates (shared with that path) against ESPHome's length limits (32 char SSID, 64 char password) plus a control-character check; empty / whitespace-only SSID, oversize values, and control characters (other than `\t`) raise `INVALID_ARGS`. `password` is optional and defaults to the empty string for open networks. |
+| `config/set_secret` | `{key, value, overwrite?}` | `{created}` | Atomically set one secret in secrets.yaml under a write lock; `overwrite=false` is create-if-absent. A duplicated key collapses to the one rewritten top-level definition, and the save is refused with `INVALID_ARGS` (file untouched) when the result wouldn't parse or doesn't resolve the key to the new value |
+| `config/set_wifi_credentials` | `{ssid, password?}` | `{}` | Set `wifi_ssid` / `wifi_password` in `secrets.yaml` via a line-based rewrite that preserves standalone and inline trailing comments and other secrets. Backs the kebab "Set up Wi-Fi" action; the create wizard's own Wi-Fi entry is persisted by `devices/create`. Validates (shared with that path) against ESPHome's length limits (32 char SSID, 64 char password) plus a control-character check; empty / whitespace-only SSID, oversize values, and control characters (other than `\t`) raise `INVALID_ARGS`. `password` is optional and defaults to the empty string for open networks. The rewrite collapses a duplicated `wifi_ssid` / `wifi_password` to one definition and is refused with `INVALID_ARGS` (file untouched) when the result still wouldn't parse. |
 
 ### Onboarding
 

esphome_device_builder/controllers/config/controller.py

@@ -16,9 +16,9 @@
     SecretsContentError,
     is_valid_secret_key,
     read_secrets_yaml,
-    validate_wifi_credentials,
+    secrets_unparsable_message,
+    set_wifi_secrets,
     write_secret,
-    write_wifi_secrets,
 )
 from ...helpers.storage import ShutdownCallback, drain_shutdown_callbacks
 from ...helpers.storage_path import resolve_storage_path
@@ -166,7 +166,8 @@ async def set_secret(
             )
         except SecretsContentError as err:
             raise CommandError(
-                ErrorCode.INVALID_ARGS, f"refusing to save invalid secrets.yaml: {err}"
+                ErrorCode.INVALID_ARGS,
+                secrets_unparsable_message("save the secret", err.problem),
             ) from err
         return {"created": created}
 
@@ -187,12 +188,9 @@ async def set_wifi_credentials(
         the next ``compile``, and preserves any other secret keys + the
         file's comments via a line-based rewrite.
         """
-        try:
-            validate_wifi_credentials(ssid, password)
-        except SecretsContentError as err:
-            raise CommandError(ErrorCode.INVALID_ARGS, str(err)) from err
-        config_dir = self._db.settings.config_dir
-        await self._db.write_secrets_locked(write_wifi_secrets, config_dir, ssid, password)
+        await set_wifi_secrets(
+            self._db.write_secrets_locked, self._db.settings.config_dir, ssid, password
+        )
         return {}
 
     @api_command("config/get_info")

esphome_device_builder/controllers/devices/controller.py

@@ -17,7 +17,7 @@
 from esphome.core import CORE
 from esphome.zeroconf import AsyncEsphomeZeroconf
 
-from ...constants import is_secrets_file
+from ...constants import SECRETS_FILENAME, is_secrets_file
 from ...helpers.api import CommandError, api_command
 from ...helpers.async_ import create_logged_task, drain_tasks, run_in_executor
 from ...helpers.build_size import BuildSizeRefreshResult
@@ -26,10 +26,9 @@
 from ...helpers.secrets_state import (
     SecretsContentError,
     read_secrets_yaml,
+    set_wifi_secrets,
     validate_secrets_content,
-    validate_wifi_credentials,
     wifi_secrets_defined,
-    write_wifi_secrets,
 )
 from ...helpers.sibling_cli import find_esphome_cmd
 from ...helpers.storage import ShutdownCallback, drain_shutdown_callbacks
@@ -680,12 +679,8 @@ async def _yaml_content_for_create(
             # that's benign — ``wifi_ssid`` / ``wifi_password`` is a shared,
             # idempotent upsert (identical to ``config/set_wifi_credentials``)
             # that the next device reuses, not per-device state.
-            try:
-                validate_wifi_credentials(ssid, psk)
-            except SecretsContentError as err:
-                raise CommandError(ErrorCode.INVALID_ARGS, str(err)) from err
-            await self._db.write_secrets_locked(
-                write_wifi_secrets, self._db.settings.config_dir, ssid, psk
+            await set_wifi_secrets(
+                self._db.write_secrets_locked, self._db.settings.config_dir, ssid, psk
             )
             ssid, psk = "", ""  # force the !secret path in the generator
             wifi_requested = True
@@ -738,6 +733,7 @@ async def _validate_rewritten_yaml_or_raise(
             packages_span=packages_span,
             packages_root=self._packages_root,
             failure_tail=failure_tail,
+            secrets_path=self._db.settings.config_dir / SECRETS_FILENAME,
         )
 
     @api_command("devices/delete")

esphome_device_builder/controllers/devices/mutations_yaml.py

@@ -15,6 +15,8 @@
     generate_device_yaml,
     generate_minimal_stub_yaml,
 )
+from ...helpers.secrets_state import secrets_problem, secrets_unparsable_message
+from ...helpers.yaml.marks import marked_paths, trim_marks
 from ...helpers.yaml.scan import block_end_index, find_block_header
 from ...models import ErrorCode
 from ..editor import ValidatorUnavailableError
@@ -139,6 +141,7 @@ async def validate_rewritten_yaml_or_raise(
     packages_span: tuple[int, int] | None = None,
     packages_root: Path | None = None,
     failure_tail: str | None = None,
+    secrets_path: Path | None = None,
 ) -> str | None:
     """
     Schema-validate *content* via the editor; raise if invalid.
@@ -163,7 +166,8 @@ async def validate_rewritten_yaml_or_raise(
     off-loop (``CORE.data_dir`` stats the disk).
 
     *failure_tail* overrides the ``INVALID_ARGS`` refusal's closing
-    sentence.
+    sentence. An error marked inside *secrets_path* (the config dir's
+    ``secrets.yaml``) refuses as ``INVALID_ARGS`` naming that file.
     """
     if editor is None:
         return None
@@ -211,7 +215,11 @@ async def validate_rewritten_yaml_or_raise(
             succeeded = True
             return warning
         _raise_validation_failure(
-            errors, action=action, on_failure=on_failure, failure_tail=failure_tail
+            errors,
+            action=action,
+            on_failure=on_failure,
+            failure_tail=failure_tail,
+            secrets_path=secrets_path,
         )
     finally:
         if not succeeded and on_error_cleanup is not None:
@@ -231,8 +239,17 @@ def _raise_validation_failure(
     action: str,
     on_failure: ErrorCode,
     failure_tail: str | None,
+    secrets_path: Path | None,
 ) -> NoReturn:
     """Raise the refusal ``CommandError`` for a failed validation."""
+    if (problem := _secrets_file_problem(errors, secrets_path)) is not None:
+        # The user's secrets.yaml is theirs to fix, whatever ``on_failure`` says.
+        if on_failure is ErrorCode.INTERNAL_ERROR:
+            # Action only: the clause names keys from the user's secrets file.
+            _LOGGER.warning(
+                "Refusing %s: errors sit in the user's secrets.yaml, not the generator", action
+            )
+        raise CommandError(ErrorCode.INVALID_ARGS, secrets_unparsable_message(action, problem))
     if on_failure is ErrorCode.INTERNAL_ERROR:
         message_tail = (
             ". Please report this with a redacted snippet of just the "
@@ -304,6 +321,19 @@ def _summarise(errors: list[str]) -> str:
     return ("; ".join(shown) + suffix).removesuffix(".")
 
 
+def _secrets_file_problem(errors: list[str], secrets_path: Path | None) -> str | None:
+    """Return the ``secrets.yaml <problem>`` clause when every error is marked inside it."""
+    if secrets_path is None or not errors:
+        return None
+    # Path equality is string-only (this runs on the event loop); both sides
+    # carry the same config_dir string, relative or not.
+    if not all(any(Path(p) == secrets_path for p in marked_paths(msg)) for msg in errors):
+        return None
+    if len(errors) == 1 and all(Path(p) == secrets_path for p in marked_paths(errors[0])):
+        return secrets_problem(errors[0])
+    return f"doesn't parse: {_summarise([trim_marks(msg) for msg in errors])}"
+
+
 def _entry_confined_to_packages(
     entry: dict, packages_span: tuple[int, int], packages_root: Path
 ) -> bool: