diff --git a/docs/reviews/SPEC-REVIEW-42-r1.md b/docs/reviews/SPEC-REVIEW-42-r1.md new file mode 100644 index 00000000..e16e6242 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-42-r1.md @@ -0,0 +1,216 @@ +# SPEC-REVIEW-42-r1 + +- Issue: #42 «[HP-ENG-01] измеряемое инженерное качество backend» +- Этап: ТЗ на ревью (PROCESS.md §2.4), полный трек (метки: P2, tests, tech-debt, + S4-spec-review; `small` отсутствует) +- Артефакт ТЗ: `docs/specs/042-backend-engineering-quality.md`, ревизия 2 + (коммит `582d673a`, docs-only, класс C) +- Заход: r1 (документов `SPEC-REVIEW-42-*.md` в репозитории не найдено — + первый заход подтверждён) +- Ревьюер: свежая сессия, без контекста написания ТЗ + +## Скоуп проверки + +ТЗ описывает одну ступень из пяти блоков: tooling-фундамент (pyproject + +requirements_test), ruff narrow, mypy strict на растущем allowlist, +coverage-механизм в CI с baseline-файлом, и формализация WS `ERROR_CODES` + +JSON-details + локализованный fallback (единственная видимая пользователю +часть). Дифф этого раунда — только сам файл ТЗ (177 добавлено / 58 удалено), +продуктовый код не тронут. + +## Как проверялось + +Ревью ТЗ на этом этапе — не код-ревью: гейты `typecheck`/`test`/`build` и +браузерные смоки к докс-диффу неприменимы (класс C, ноль файлов `src/**` или +`custom_components/**/*.py` в этом коммите). Вместо этого перепроверялись +фактические утверждения ТЗ по текущему коду `dev` — то, что обычно и выдаёт +догадку, поданную как факт: + +- `python3 -m ruff check custom_components/houseplan --select E,F,B,I + --target-version py313 --statistics` → **333 (E501 291, B023 17, I001 13, + F401 6, E731 2, F841 2, B905 2)** — совпадает с цифрами ТЗ дословно; +- `custom_components/houseplan/quality_scale.yaml` → ровно 4 `todo`: + `test-coverage`, `docs-troubleshooting`, `docs-examples`, `strict-typing` — + совпадает с заявлением владельца в issue; +- `docs/USER-GUIDE.md` §22 Troubleshooting существует (строка 1034), + `docs/USER-GUIDE.ru.md` раздела «Troubleshooting»/«Устранение неполадок» не + содержит вовсе — совпадает; +- `grep -c '"backup.error\.' src/i18n/en.json` → **26** ключей — совпадает; +- `.github/workflows/mutation-gate.yml:67` действительно дублирует строку + `pip install pytest voluptuous pytest-homeassistant-custom-component + home-assistant-frontend` из `validate.yml` — совпадает; +- прочитан весь путь ошибки на фронте (`src/houseplan-card.ts:9497-9536` + `_errText`, `src/houseplan-editor-runtime.ts:8002-8010` `_backupErrorText`) + и все точки эмиссии `send_error` (`custom_components/houseplan/ + websocket_api.py`, 54 вызова) плюс классы с публичным `.code` + (`validation.py`, `junction_limits.py`, `import_export.py`) — см. находку + Medium 1. + +Не запускался: `npm run typecheck/test/build`, `check-docs`, `model-invariants`, +браузерные смоки, `pytest tests_backend`, mypy/coverage прогон — см. «Чего не +проверял». + +## Находки + +### Medium 1 (в скоупе) — AC5 не покрывает коды, эмитируемые не литералом в `send_error(...)` + +**Файл:** `docs/specs/042-backend-engineering-quality.md`, раздел «5. WS error +contract + доки» и «AC5». + +**Формулировка ТЗ:** «ERROR_CODES ⊇ все коды send_error (скан исходника): +каждый литерал `send_error(...)`-кода ∈ ERROR_CODES». Как метод верификации +это подразумевает статический скан аргументов вызовов `send_error(...)` на +литеральные строки. + +**Почему это не выполнимо как написано.** Как минимум четыре класса ошибок +несут код не литералом внутри `send_error(...)`, а через `err.code`, +прочитанный в обработчике (`websocket_api.py:189` `send_error(msg_id, +err.code, err.message)`; `:1370` и `:1747` `send_error(msg["id"], err.code, +str(err))`): + +- `validation.py:43-54` `OpeningPassageError.code = "invalid_passage_fields"` + и `validation.py:63-72` + `PartitionOpeningJambMarginError.code = "invalid_partition_opening_jamb_margin"` + — это ровно те два кода, вокруг которых построен весь блок 5 (JSON-details). + Скан по литералам `send_error(...)` их не найдёт — они читаются из + атрибута класса, а не передаются строкой в месте вызова; +- `validation.py:57-60` `PartitionOpeningHostError.code = + "invalid_partition_opening_host"` и `validation.py:81-84` + `WallModelClientOutdatedError.code = "wall_model_client_outdated"` — то же; +- `validation.py:35-40` `MarkerControlError.__init__(self, code, message)` — + код передаётся аргументом конструктора; часть литеральна + (`"duplicate_marker_control"`, `"marker_control_self"`, + `"invalid_value_badge"` и др., `validation.py:824-968`), часть собрана + f-строкой из `prefix` с ровно двумя значениями + (`validation.py:818`: `"value_badge"` / `"value_source"`) → + `value_badge_marker_missing`, `value_source_marker_missing`, + `value_badge_marker_not_light`, `value_source_marker_not_light` + (`validation.py:847,849`); +- `junction_limits.py:56` `JunctionLimitError.code = f"junction_limit_{rule}"` + — `rule` пробегает конечное множество ключей П1–П4 (`junction_limits.py:427-430`), + но опять не литерал внутри `send_error(...)`. + +Итого не меньше дюжины уже существующих кодов — включая **оба** кода, ради +которых написан блок 5 — невидимы для скана, читающего только литералы в +`send_error(...)`. Реализация AC5 «в лоб» даст зелёный контракт-тест, который +ничего не доказывает для этих кодов: они не попадут в `ERROR_CODES`, останутся +без ключа `backup.error.`, и `_errText` продолжит показывать сырой +`e.message` (`houseplan-card.ts:9528`) ровно для того класса ошибок, который +issue называет проблемой. Это технический, не продуктовый вопрос (какой метод +верификации использовать), поэтому решаю его в вердикте, а не выношу +владельцу. + +**Как чинится в скоупе:** AC5/раздел 5 должны явно назвать способ, которым +скан достаёт коды из `err.code` — либо (а) перечислить классы-источники +(`OpeningPassageError`, `PartitionOpeningHostError`, +`PartitionOpeningJambMarginError`, `WallModelClientOutdatedError`, +`MarkerControlError`, `JunctionLimitError`, `ImportFailure`) и извлекать их +`code`-литералы/шаблоны статическим разбором модуля, либо (б) явно сузить +AC5 до кодов-литералов в `send_error(...)` и отдельно перечислить +раскрытые f-строкой/классом семейства как «покрыты общим локализованным +fallback по коду, без выделенного ключа» — тогда `_errText`/`_backupErrorText` +обязаны фактически падать в этот fallback для них, а не в сырой `e.message` +(что снова упирается в порядок проверок в `_errText`, см. Medium 2). + +### Medium 2 (в скоупе) — раздел «i18n» (обязателен по PROCESS.md §7.1) отсутствует; повторное использование `err.code` не зафиксировано + +**Файл:** тот же, ТЗ целиком — раздела с заголовком «i18n» нет ни одного. + +DoR §2.5 требует «i18n: ключи en + ru перечислены». Блок 5 обещает: +«неизвестный код → общий локализованный текст + код» — но не говорит, +это НОВЫЙ ключ или переиспользование существующего. + +Проверка кода показывает, что подходящий ключ уже есть и уже переведён: +`src/i18n/en.json:338` `"err.code": "code {code}"`, +`src/i18n/ru.json:338` `"код {code}"` (де/фр тоже переведены). Реальная +причина текущего дефекта не в отсутствии такого ключа, а в порядке проверок +внутри `_errText` (`houseplan-card.ts:9528`: `if (e.message) return +e.message;` стоит РАНЬШЕ ветки `err.code`) — значит для любой ошибки, где +бэкенд шлёт одновременно код и `message`, независимо от известности кода, +сейчас показывается сырое английское `message`. AC6 («неизвестный код → +локализованный fallback») не сможет быть проверен юнит-тестом однозначно, +пока ТЗ не решит: (а) переиспользуется `err.code`/`err.unknown` без новых +ключей — тогда раздел i18n тривиален («новых ключей нет, порядок проверок в +`_errText` меняется на code-first») — либо (б) вводится новый текст — тогда +нужны конкретные en+ru строки. Без явного выбора ревьюер кода не сможет +сверить AC6 с намерением автора, а автор рискует написать тест под +собственную догадку, которую я не смогу отличить от решения. + +**Как чинится в скоупе:** добавить короткий раздел «i18n» с явным решением +(рекомендация — вариант (а), ключ уже есть и уже переведён на 4 языка) и +одной строкой описать смену порядка проверок в `_errText` +(code-first → message → error → JSON). + +## Low (снимаю с записью, не блокирует) + +Разделы «UX» и «Модель данных и миграция», формально обязательные по +PROCESS.md §7.1 как отдельные заголовки, в файле не оформлены как таковые — +их содержание фактически присутствует, но разбросано («Что человек увидит до +и после» покрывает UX: новых диалогов/интеракций нет, меняется только текст; +«DoR-примечания» закрывает миграцию: единственное затронутое поле — формат +`message` двух кодов, обратная совместимость на одну бету). Контент по +существу верный и достаточный, поэтому не поднимаю до Medium — прошу +консолидировать при следующей правке ради дословного соответствия §7.1, но +это не требует нового цикла ради одного этого пункта. + +## Что проверено и корректно + +- **Сценарий** и **«что человек увидит до и после»** — на месте, продуктовые, + без терминов реализации; соответствуют J-рядам SCOPE.md лишь косвенно + (инженерное качество — не отдельная строка core user jobs), но решение + вести эту работу и её ценностная оценка (3/10 пользователю, 9/10 разработке) + уже приняты владельцем в комментариях issue — не переоткрываю; +- **Скоуп/не-скоуп** разделены явно, «следующая ступень» зафиксирована + текстом, а не памятью; +- **Контракт поведения**: код и структура успешных ответов не меняются; + единственное видимое изменение — формат `message` двух кодов на JSON, + риск для рассинхронизированной пары фронт/бэк одной беты назван и обоснован + явно, а не спрятан; +- **AC1–AC4, AC6, AC7** — однозначны, у каждого назван способ доказательства + (CI/локально/юнит), и по каждому видно, как тест умеет упасть (AC1 — + «подмена baseline на большее число», AC2 — «удаление homeassistant из шага + установки», AC4 — «контракт-тест падает при сужении списка», AC6 — м1/м2 + мутанты в разделе «План автотестов»); +- **Риски** называют главный поведенческий риск (B023-фиксы) и версийный + разъезд (3.10 песочница / 3.13 CI) без сокрытия; +- **Откат** — `git revert`, без потери данных, явно; +- **Release-артефакты** — оба changelog, `ARCHITECTURE.md`, `USER-GUIDE.ru` + §Troubleshooting — названы; +- **Принятые предположения** — присутствуют отдельным блоком, включая ту же + идею про `backup.error.` как общее пространство кодов, которую + находка Medium 2 просит явно продолжить в раздел i18n; +- порог coverage 90→95 сознательно вынесен в отдельные trivial-issues — + разумно, механизм этой ступени уже будет их принудительно проверять. + +## Чего не проверял и почему + +- `npx tsc --noEmit`, `npm test`, `npm run build` + сверка копий бандла, + `node scripts/check-docs.mjs`, `node scripts/model-invariants.mjs` — диф + этого раунда состоит из одного файла `docs/specs/042-*.md` (класс C), в + `src/**` и `custom_components/**/*.py` изменений нет; прогон гейтов кода + на неизменном коде не даёт сигнала по существу ТЗ и не входит в предмет + ревью ТЗ (PROCESS.md §2.4 против §2.7); +- браузерные смоки, `golden:verify`, `pytest tests_backend`, performance — + та же причина, плюс на этом этапе AC не привязаны к конкретному коду, + который можно было бы прогнать; +- фактическое покрытие 89.1% и «pure 240/0» из измерений владельца — не + переснимал (нужен рабочий HA-harness/`pytest-cov`, которых в песочнице нет + и установка которых для разового замера ушла бы за рамки ревью ТЗ). Считаю + косвенным подтверждением точное совпадение независимо перепроверенных + ruff-цифр (333/291/17/13/6/2/2) — метод измерения автора доверия + заслуживает; +- осуществимость конкретного стартового allowlist mypy strict (`const`, + `projection`, `coordinate_canonicalization`, `frontend_asset_manifest`, + `junction_limits`, `plans` + часть `validation`/`wall_segment_model`/ + `geometry_migration`) — ТЗ само откладывает точный финальный список «по + факту зелени» в handoff, это законное «assumed, решается в реализации», не + спец-пробел. + +## Итог + +Механизм в целом обоснован и предметен — почти все числа и ссылки в ТЗ +проверяются на HEAD `dev` дословно, что необычно хорошо для спека такого +объёма. Возврат — по двум находкам Medium в скоупе задачи: AC5 нужно +дотянуть до кодов, приходящих через `err.code` (включая оба флагманских кода +блока 5), и явно решить/записать пункт i18n. Обе решаются добавлением текста +в тот же файл, без пересмотра архитектуры и без нового цикла владельца.